uthash: check findstr pointer validity in HASH_FIND_STR

[mattmart3]

This to avoid the uthash_strlen to segfault with a null pointer

[Quuxplusone]

Hi @matteomartelli! I don't think this patch makes sense: As-is, it changes HASH_FIND_STR to avoid looking for null pointers (which are not "strings"), but it doesn't change HASH_ADD_STR to avoid adding them, or HASH_REPLACE_STR to avoid replacing them. So let's pretend you changed all three places.

Each change has the same form: "If the 'string' to find is actually NULL, then pretend it was not found." My first thought was that you could achieve this by just overriding uthash_strlen to return 0 for null pointers; but as I write this, I see that that would treat NULL as equivalent to "", which is different.

Still, I don't think it makes sense to slow down every user of HASH_FIND_STR just for the benefit of the (rare — until this week, literally non-existent) user who wants to treat NULL as "a string, but one which is never found in the table." That user should just write an if outside of the uthash stuff:

// Invariant: ptr is either a valid C string, or NULL
ptr = (cond ? "hello world" : NULL); ...

Node *found = NULL;
if (ptr != NULL) {
    HASH_FIND_STR(mytable, ptr, found);
}

An alternative approach would be to use the "null object pattern":

extern const char MYNULL[] = "(nil), specifically, a string whose value I promise will never ever appear in the table";

// Invariant: ptr is either a valid C string, or MYNULL (which is also a string)
ptr = (cond ? "hello world" : MYNULL); ...

Node *found = NULL;
HASH_FIND_STR(mytable, ptr, found);

So I'm inclined to close this.

[mattmart3]

Hi @Quuxplusone , thanks for the quick reply.
I agree that in case the same safety check should be added to HASH_ADD_STR and HASH_REPLACE_STR, my bad for having missed them.
The rationale is to provide a safer interface such as the strnlen_s which checks if the string is a null pointer and return 0 in that case.
However, I understand the performance concerns. What about a configuration definition such as HASH_SAFE_STRINGS, that if defined would instead use a safe strlen wrapper or null safety checks into the HASH_*_STR macros?
It might have a higher impact than I expect, so I understand if you think that's not worth it given the rare use case.

[Quuxplusone]

The rationale is to provide a safer interface such as the strnlen_s which checks if the string is a null pointer and return 0 in that case. [...] What about a configuration definition such as HASH_SAFE_STRINGS, that if defined would instead use a safe strlen wrapper

But that is just literally -Duthash_strlen(x)=strnlen_s(x, SIZE_MAX), right? You don't need uthash to change at all, if that's all you want to do. (But as I said, that makes NULL equivalent to "", which isn't quite the same as what you originally proposed.)

[mattmart3]

Sorry for the confusion, as you pointed out using strnlen_s() would make NULL pointer equivalent to "", and thus HASH_*_STR would threat NULL pointers as valid keys, which is not what I would achieve: for instance HASH_ADD_STR accesses to strfield[0] which would break with NULL pointers anyway. Thus either the check is done in a similar way as my first proposal with an additional define to avoid the check by default, or the check must be done externally by the caller as you suggested.
To conclude, if you think an additional optional define to add the NULL pointer check doesn't make sense I am fine to close this PR, otherwise I'll try to edit my proposal in that way.

[Quuxplusone]

for instance HASH_ADD_STR accesses strfield[0] which would break with NULL pointers anyway

Well, that probably depends on one's reading of the Standard. My impression is that in practice it's totally fine to write &x[0] or &*x even when x is null: the compiler will make &*x equal to x regardless of whether x is dereferenceable. I don't know what the official ruling is (e.g. whether some sanitizer might trigger on that).

(Notice that HASH_ADD_STR isn't evaluating add->strfield[0]; it's taking its address.)

But yeah, I'm unlikely to take any patches in this area. I think if the user wants to store string keys they should use HASH_ADD_STR as-is; and if they want to store keys-that-may-or-may-not-be-strings they should not use HASH_ADD_STR at all (because NULL isn't a string).

[mattmart3]

I understand your point. Thanks for having addressed it.