Page MenuHomeFreeBSD

elfutils: Fix port build after recent basename() API break
ClosedPublic

Authored by cem on Aug 3 2016, 4:59 AM.

Details

Summary

Port assumes that basename() will not modify valid paths and uses basename() on
const char strings repeatedly, as well as assuming the result points into the
argument. The most correct way to emulate this is to just __DECONST() all
invocations from this library before passing them to libc basename.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4693
Build 4747: arc lint + arc unit

Event Timeline

cem retitled this revision from to elfutils: Fix port build after recent basename() API break.Aug 3 2016, 4:59 AM
cem updated this object.
cem edited the test plan for this revision. (Show Details)
cem updated this revision to Diff 18988.
cem removed a subscriber: mat.
bdrewery edited edge metadata.Aug 3 2016, 4:36 PM

"as well as assuming the result points into the argument". You reviewed the uses of basename() here and are sure that the internal change in the future to basename() to modify the argument will be safe?

cem added a comment.Aug 3 2016, 6:07 PM

"as well as assuming the result points into the argument". You reviewed the uses of basename() here and are sure that the internal change in the future to basename() to modify the argument will be safe?

I didn't review all of them individually. This was written on Linux, which doesn't have a const char * basename. Instead, it has two different versions (I know, what the hell Linux).

There are two different versions of basename() - the POSIX version described above, and the GNU version, which one gets after

    #define _GNU_SOURCE         /* See feature_test_macros(7) */
    #include <string.h>

The GNU version never modifies its argument, and returns the empty string when path has a trailing slash, and in particular  also
when it is "/".  There is no GNU version of dirname().

With glibc, one gets the POSIX version of basename() when <libgen.h> is included, and the GNU version otherwise.

None of the sources include libgen.h; they include string.h, and somehow COMPILE includes DEFS which gets -D_GNU_SOURCE from somewhere. So I suspect they are assuming the GNU version of basename, which actually behaves more like a char *basename(const char *) than our future one will. So I guess this needs to be reworked.

cem edited edge metadata.Aug 3 2016, 6:13 PM
cem updated this revision to Diff 19015.

Replace libc POSIX-style basename(3) with handwritten GNU-style basename(3).

cem added a comment.Aug 5 2016, 4:55 PM

Hey all, reviewer timeout is approaching. Thanks!

In D7404#154414, @cem wrote:

Hey all, reviewer timeout is approaching. Thanks!

I don't think there is an approval timeout for people without a proper bit.

This looks OK but lacks the full POSIX support (http://pubs.opengroup.org/onlinepubs/009695399/functions/basename.html), such as "If the string pointed to by path consists entirely of the '/' character, basename() shall return a pointer to the string "/". " and "If path is a null pointer or points to an empty string, basename() shall return a pointer to the string "."."

To get this behavior you could just copy what basename(3) is from libc right now and modify the prototype to be what it was before the change:

char *
eu_basename(const char *path)
{
        static char *bname = NULL;

        if (bname == NULL) {
                bname = (char *)malloc(MAXPATHLEN);
                if (bname == NULL)
                        return (NULL);
        }
        return (basename_r(path, bname));
}
bdrewery edited edge metadata.Aug 5 2016, 5:17 PM
bdrewery accepted this revision.
This revision is now accepted and ready to land.Aug 5 2016, 5:17 PM
cem added a comment.Aug 5 2016, 5:30 PM
In D7404#154414, @cem wrote:

This looks OK but lacks the full POSIX support (http://pubs.opengroup.org/onlinepubs/009695399/functions/basename.html), such as "If the string pointed to by path consists entirely of the '/' character, basename() shall return a pointer to the string "/". " and "If path is a null pointer or points to an empty string, basename() shall return a pointer to the string "."."

Sure, but this isn't POSIX basename(), it's GNU basename(). It exists intentionally to not do those things. The previous comment explains this a little bit.

To get this behavior you could just copy what basename(3) is from libc right now and modify the prototype to be what it was before the change:

That would defeat the point. That's what I had in the previous version of the patch; the previous comment explains why that's wrong for this library.

This revision was automatically updated to reflect the committed changes.
In D7404#154423, @cem wrote:
In D7404#154414, @cem wrote:

To get this behavior you could just copy what basename(3) is from libc right now and modify the prototype to be what it was before the change:

That would defeat the point. That's what I had in the previous version of the patch; the previous comment explains why that's wrong for this library.

Well no, you were casting to use the basename(2) in libc now which may change in the future. My suggestion was a future-proof one.

cem added a comment.Aug 5 2016, 5:40 PM

Well no, you were casting to use the basename(2) in libc now which may change in the future. My suggestion was a future-proof one.

Sure, but both were wrong :-).