Hello Alexander, On 27.01.2022 21:12, Alexander Dahl wrote:
However this is not a such case. MHD never have guaranteed tolerating of NULL in this function. The data must be a valid C string. The NULL pointer is definitely not a valid C string.Well, I could not tell based on the API documentation, what's intended behaviour here.
Pointer to char is always C sting in C, unless another type is explicitly documented (like pointer to a single char).
NULL is not a valid C string.Even if something is unclear from documentation, it is called "undefined behaviour". Undefined behaviour should be avoided as it may work on one platform or version and may not work on another platform or version.
MHD would (silently) ignore bad parameters with your patch. This may mask some bug in code that use MHD_add_response_header() function so while this patch may look useful for production code, actually it makes situation even worse as it may give false feeling of correct functioning instead of revealing bug as soon as possible.Well, my patch just restores the behaviour of libmicrohttpd from before 185f740e0684, where MHD_add_response_header() returned what add_response_entry() returns. That function add_response_entry() checks for (response == NULL) and silently returns MHD_NO in that case. Intended or not, changeset 185f740e0684 broke that behaviour, and it did not mention in commit message nor in code itself.
If behaviour is undefined, it cannot be broken.You should not expect that NULL will be processed as a valid C string. On the other hand, it was not guaranteed that MHD would segfault on NULL.
Hypothetical situation: =============================== MHD_add_response_header (r, "System-Command", "sudo rm -rf / --no-preserve-root"); char *behaviour = strdup ("no-execute, check-syntax-only"); MHD_add_response_header (r, "Modify-Behavior", behaviour); free (behaviour); MHD_queue_response (c, MHD_HTTP_OK, r); ============================== Very typical mistake: no checks for returned values. The code may work fine, but at some moment, due to some leak in another place, strdup() fail. With your patch second header would not be added, but response would be sent. I think it's better to crash earlier in such situation than trying to fix automatically wrong parameters.You're absolutely right. Users of your library must check return values, no doubt about that.
Also users of the library should provide correct data.
From the description of the issue in your library, it's clear than you was able to find another error (return 200 instead of 404) with wrong filename only when MHD stopped tolerating NULL. If MHD would not tolerate NULL initially (which was not intentional behaviour), you would be able to find and fix this bug earlier.It's not my library, I'm just a user. And I'm sorry, I'm confused now. What is the intentional behaviour of libmicrohttpd?
Intentional behaviour is correct processing of correct data. So tricky and/or complicated combination of strings are detected on run-time, but fundamental types expected to be correct, like strings are zero-terminated and in valid readable memory area.
I mean, libmicrohttpd detecting NULL and returning MHD_NO tells me something went wrong at runtime, if I check for return value, which I should do.
Yes, you should check the return value as well as you should provide valid C strings. When application pass NULL as a C string, it's clear indication that something goes terribly wrong in the application, right? The best thing that you can do in such situation is to abort the broken application, before it overwrites some valid data with some invalid data or does other horrible unexpected things.
Checking for return value is pointless, however, if libmicrohttpd crashes before returning anything.
That's correct. But if application supplies such obvious invalid input, there is a high chance that return value is not checked as well, just because most probably the invalid input was the return value from another function that was not checked before.
Crash/segfault is your friend when something is badly broken. Masking obvious problems does not help debugging and testing and makes things worse in production. On the other hand "libmicrohttpd" is still "micro". The library core without optional parts still could be very small. So library expects that parameters have valid types. Otherwise we should bloat up the code with checks for every single parameter validity, including readability of pointed memory region.
-- Best Regards, Evgeny
OpenPGP_signature
Description: OpenPGP digital signature
