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

[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.

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