Hello Evgeny,
thanks for your detailed answer. I read that basically as "I can not assume
libmicrohttpd checks user input" in general, but I have some remarks below.
Am Freitag, 28. Januar 2022, 10:31:26 CET schrieb Evgeny Grin:
> 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.
While that's certainly true, I did not talk about 'char *' or 'const char *'
but about 'struct MHD_Response *response' which is a different type. Maybe
the same argumentation applies, though.
> 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.
I did not expect that.
> On the other hand, it was not guaranteed that MHD would segfault on NULL.
Nor was guaranteed MHD would check parameters. But it actually did check
before 185f740e0684 and crashed after. ¯\_(ツ)_/¯
> >> 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.
True.
> >> 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.
I actually provided valid C strings for both 'header' and 'content'. It was
'struct MHD_Response *response' which was NULL.
> 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.
I think this tends to be a question of design philosophy. Some developers
expect libraries to never segfault.
> 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.
Well, then you could drop some checks from 'add_response_entry()' in source
file "src/microhttpd/response.c", right? Especially the first three of them?
static enum MHD_Result
add_response_entry (struct MHD_Response *response,
enum MHD_ValueKind kind,
const char *header,
const char *content)
{
struct MHD_HTTP_Header *hdr;
if ( (NULL == response) ||
(NULL == header) ||
(NULL == content) ||
;-)
Don't take all this as pro argumentation for my patch. I'm perfectly fine
with dropping it, if it does not comply with the design philosophy of
libmicrohttpd.
Greets
Alex
