src/libudev/conf-files.c: fix bug of using basename

[xfan1024]

BASENAME(3) Linux Programmer's Manual say:

These functions may return pointers to statically allocated memory which may be overwritten by subsequent calls.

Using basename return value as key of hashmap is not safe, it may cause that hashmap_put always return -EEXIST if hash collision happen.

Using basename return value as strcmp first and second parameters may always return 0.

[lu-zero]

basename_r is non-standard so doesn't look like we can have a simpler workaround.

[xfan1024]

basename_r is non-standard so doesn't look like we can have a simpler workaround.

Is it acceptable using custom basename_internal?

[bbonev]

src/shared/conf-files.c:145: fh = hashmap_new(&string_hash_ops);

string_hash_func calculates the hash based on the string value, not the pointer, so that part is safe

But this one is not: return strcmp(basename(s1), basename(s2)); as long as the second call may return the same memory as the first one and overwrite the contents... Also it is mentioned that basename may modify its argument and then a real mess would happen.

Instead of looping over all path components, what about using strrchr?

[ArsenArsen]

Holding an implementation of a function is quite fine, though, as noted, strrchr might save on some code in there.

[bbonev]

While at it, maybe it is a good idea to also include this:

src/shared/util.c:1728: fn = basename((char*)p);

It discards the const modifier and violates POSIX...

[bbonev]

@xfan1024 What about that:

const char *eudev_basename(const char *filename) {
    char *p=strrchr(filename,'/');

    if (p)
        return p+1;
    return filename;
}

Because this is used in more than one place, it would best to put it in src/shared/util.c and the prototype in util.h

[xfan1024]

@xfan1024 What about that:

const char *eudev_basename(const char *filename) {
    char *p=strrchr(filename,'/');

    if (p)
        return p+1;
    return filename;
}

Because this is used in more than one place, it would best to put it in src/shared/util.c and the prototype in util.h

Agree it.

[xfan1024]

src/shared/conf-files.c:145: fh = hashmap_new(&string_hash_ops);

string_hash_func calculates the hash based on the string value, not the pointer, so that part is safe

hashmap hold string's pointer not string's value. hashmap_put have 2 steps to check the key is exists, first calculate string's hash code, second strcmp all string in hashmap which have the same hash code, that equivalent to strcmp(basename(s1), basename(s2)), so I think that also unsafe.

[bbonev]

There is one more invocation - src/shared/util.c:1728 Please fix that too and it's good to merge

[xfan1024]

There is one more invocation - src/shared/util.c:1728 Please fix that too and it's good to merge

Done