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

[cem]

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.

[cem]

http://beefy4.nyi.freebsd.org/data/head-amd64-default/p419462_s303652/logs/elfutils-0.163_6.log

[bdrewery]

"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]

In D7404#153986, @bdrewery wrote:

"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]

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

[cem]

Hey all, reviewer timeout is approaching. Thanks!

[bdrewery]

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));
}

[cem]

In D7404#154419, @bdrewery wrote:

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.

[bdrewery]

In D7404#154423, @cem wrote:

In D7404#154419, @bdrewery 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]

In D7404#154426, @bdrewery wrote:

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 :-).