Re: [libmicrohttpd] [PATCH] MHD_add_response_header: Check on passed nullptr

[Alexander Dahl]

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