[BUG] unpredictable results in file_response with filename pointing into the void

[LeSpocky]

Prerequisites

• Put an X between the brackets on this line if you have checked that your issue isn't already filed: https://github.com/search?l=&q=repo%3Aetr%2Flibhttpserver&type=Issues

Description

When creating an object of class httpserver::file_response with a filename not related to an actually existing file, the behavior of the application is more or less unpredictable depending on which version of libmicrohttpd is used. We saw HTTP responses with status 200 and an empty data part, but the app might as well segfault.

Steps to Reproduce

1. Create an instance of class httpserver::file_response with a filename pointing to a non existent file
2. Use that object as a response for an actual request

Expected behavior: webserver returns a sensible HTTP status code like 500 (Internal Server Error) or better 404 (Not Found).

Actual behavior: webserver crashes (segfault), does not answer, or even returns code passed to constructor of file_response

Reproduces how often: always

Versions

• OS version
  • Debian GNU/Linux 10 (buster)
  • custom built embedded firmware
• libhttpserver version: master, self compiled
• libmicrohttpd version: multiple self compiled from v0.9.53 up to v0.9.73

If you have problems during build: build is fine

Additional Information

What happens is httpserver::file_response::get_raw_response() might return a nullptr which is then passed on to http_response::decorate_response() which itself calls some libmicrohttpd functions like MHD_add_response_header() or MHD_add_response_footer() which might or might not crash depending on version.

Will show a unit test reproducing this.

[LeSpocky]

See #256 for the promised unit test.

[LeSpocky]

FTR: There's an ongoing discussion about how libmicrohttpd should behave in case someone passes them a nullptr: https://lists.gnu.org/archive/html/libmicrohttpd/2022-01/msg00018.html

[LeSpocky]

FTR: There's an ongoing discussion about how libmicrohttpd should behave in case someone passes them a nullptr: https://lists.gnu.org/archive/html/libmicrohttpd/2022-01/msg00018.html

From that discussion it becomes clear libhttpserver should not assume MHD checks if some argument is a nullptr. As a MHD user we must ensure we do not pass nullptr to libmicrohttpd.

[etr]

Point taken - I agree with you (and them) that we should check on this side of the wall and libhttpserver should guarantee it doesn't produce any unexpected behavior.

What I am currently pondering is whether the file_resource should handle this fully (i.e. providing a mechanism to handle this end-to-end with a 404 for example) or just delegate this responsibility to whoever uses the library. I'm leaning towards the latter as it.

In the latter case, the library returning by default a 500 might just be the right thing to do as the responsibility to make sure that the file indeed exist and handle it gracefully would be delegated its clients.

[LeSpocky]

Point taken - I agree with you (and them) that we should check on this side of the wall and libhttpserver should guarantee it doesn't produce any unexpected behavior.

libhttpserver is somewhat special here, because it is a user towards libmicrohttpd on one side and provides itself an interface to other users on the other side.

What I am currently pondering is whether the file_resource should handle this fully (i.e. providing a mechanism to handle this end-to-end with a 404 for example) or just delegate this responsibility to whoever uses the library. I'm leaning towards the latter as it.

I did not have a good idea for an elegant solution to fully handle it in libhttpserver, otherwise I would have proposed that already.

In the latter case, the library returning by default a 500 might just be the right thing to do as the responsibility to make sure that the file indeed exist and handle it gracefully would be delegated its clients.

I think I could live with that. This is more flexible for users of libhttpserver and testing if a file exists in my app is trivial. This behaviour could be mentioned in the API docs of file_response then?