Hello Evgeny,
Am Thu, Jan 27, 2022 at 08:15:37PM +0300 schrieb Evgeny Grin:
> Thanks for the patch.
> Breaking API is always a bad idea, unless API itself is broken badly by
> design.
> 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.
> 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.
> 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.
> 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?
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.
Checking for return value is pointless however, if libmicrohttpd
crashes before returning anything.
Greets
Alex
>
> --
> Evgeny
>
>
> On 27.01.2022 14:10, Alexander Dahl wrote:
> > The response argument is passed to `add_response_entry()` eventually
> > which does a check on NULL. This was done without accessing struct
> > members of *response* in the past, however since 185f740e0684 ("allow
> > clients to override sanity check for content-length header") an access
> > to response->flags leads to a segfault.
> >
> > This was spotted when building an app with libhttpserver which currently
> > might pass a nullptr to `MHD_add_response_header()`, see the bug report
> > over there for details.
> >
> > Link: https://github.com/etr/libhttpserver/issues/255
> > Fixes: 185f740e0684 ("allow clients to override sanity check for
> > content-length header")
> > Signed-off-by: Alexander Dahl <a...@thorsis.com>
> > ---
> >
> > Notes:
> > Hello everyone,
> > I discovered this when working with libhttpserver [1] which currently
> > does not check some return codes and thus ends up passing a null
> > pointer. This was no problem against version 0.9.62-1 from the debian
> > package, but is against recent 0.9.75. I'm working on fixing that
> > potentially harmful behaviour of the other lib, but I think the check
> > here is valuable in itself, because it prevents libmicrohttpd to
> > segfault.
> > Greets
> > Alex
> > [1] https://github.com/etr/libhttpserver
> >
> > src/microhttpd/response.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/src/microhttpd/response.c b/src/microhttpd/response.c
> > index ca3639f4..2a8b3cbe 100644
> > --- a/src/microhttpd/response.c
> > +++ b/src/microhttpd/response.c
> > @@ -494,6 +494,9 @@ MHD_add_response_header (struct MHD_Response *response,
> > const char *header,
> > const char *content)
> > {
> > + if (response == NULL)
> > + return MHD_NO;
> > +
> > if (MHD_str_equal_caseless_ (header, MHD_HTTP_HEADER_CONNECTION))
> > return add_response_header_connection (response, content);
> >
> > base-commit: 1b1361e4c6e07a74e1a70f96fc570510aaa36815
