File response #256

LeSpocky · 2022-01-27T10:58:23Z

Requirements for Contributing a Bug Fix

Identify the Bug

Description of the Change

Adds a unit test highlighting the bug, no real fix so far.

Alternate Designs

This snippet at least fixes the webserver to return a 500 Internal Server Error:

diff --git a/src/webserver.cpp b/src/webserver.cpp
index a1392bd..4c4a034 100644
--- a/src/webserver.cpp
+++ b/src/webserver.cpp
@@ -623,6 +623,10 @@ MHD_Result webserver::finalize_answer(MHD_Connection* connection, struct details
     try {
         try {
             raw_response = mr->dhrs->get_raw_response();
+            if (raw_response == nullptr) {
+                mr->dhrs = internal_error_page(mr);
+                raw_response = mr->dhrs->get_raw_response();
+            }
         } catch(const std::invalid_argument& iae) {
             mr->dhrs = not_found_page(mr);
             raw_response = mr->dhrs->get_raw_response();

The unit test just fails different then, but that confirms the unit test itself is ok:

Running test (25): file_serving_resource_missing
[ASSERT FAILURE] (../../test/integ/basic.cpp:923) - error in "file_serving_resource_missing": 500!=404
- Time spent during "file_serving_resource_missing": 0.668945 ms

Possible Drawbacks

n.a.

Verification Process

Tested against multiple different version of libmicrohttpd.

Release Notes

n.a.

LeSpocky · 2022-01-28T20:07:46Z

As discussed in #255 that diff could be a solution already. The unit test could test on HTTP status NOT equal to 200 instead? Or test equal to 500?

If `http_response::get_raw_response()` returns nullptr instead of a valid `MHD_Response*` for whatever reason, that pointer would be passed on to `http_response::decorate_response()` and `http_response::enqueue_response()` eventually, leading to different API calls to libmicrohttpd with NULL as argument `struct MHD_Response *response`. MHD does not guarantee any form of behaviour for invalid input, so we have to consider it undefined behaviour and avoid passing invalid input to MHD. HTTP status 500 (Internal Server Error) is returned for consistency with surrounding error handling, we don't know what caused `http_response::get_raw_response()` to return nullptr, so we can not give a better answer here. Fixes: etr#255

Both `open()` and `lseek()` might fail depending on how `filename` is set in object of class `httpserver::file_response()`. In the case of a missing file *fd* got -1 and lseek set *size* (which had the wrong type btw.) to 0xffffffff aka (off_t) -1. Passing an invalid file descriptor and a massively huge size value on to `MHD_create_response_from_fd()` might lead to unpredictable results depending how well libmicrohttpd treats such invalid values. Note: Before f9b7691 ("Use MHD_create_response_from_fd for files") `httpserver::http::load_file()` was used, which throws an exception in case a file can not be opened successfully. That exception would have lead to returning HTTP status 500 (Internal Server Error). References: etr#255

The constructor of class `httpserver::file_response` can be called with a `filename` pointing into the void, to a file which does not actually exist. The webserver should fail predictably in that case. References: etr#255

LeSpocky · 2022-01-29T11:05:36Z

I force pushed a possible solution as discussed in #255, but did not change description etc. of this PR, yet. Should I?

LeSpocky · 2022-01-29T11:11:46Z

src/file_response.cpp

-    size_t size = lseek(fd, 0, SEEK_END);
+    if (fd == -1) return nullptr;
+
+    off_t size = lseek(fd, 0, SEEK_END);


Type of size taking return value of lseek() should be off_t. That was introduced wrong with f9b7691. Could be fixed in a separate commit or just fixed here en passant.

LeSpocky · 2022-01-31T14:34:49Z

I just discovered another problem: if that filename points to a directory instead of a file … 🙄

etr · 2022-01-31T16:43:59Z

I just discovered another problem: if that filename points to a directory instead of a file …

We should just manage this in the same way (direction = invalid) and express it in the documentation making it clear that only existing files (not directories) are acceptable.

Type of size taking return value of lseek() should be off_t. That was introduced wrong with f9b7691. Could be fixed in a separate commit or just fixed here en passant.

Given the broad changes that are going to happen in this method, I think we can just do it in this change.

It was possible before to pass a path to a directory, or a to a device file, or to basically any path. `open()` would happily open it. In case of a directory, `lseek()` returns LONG_MAX (0x7FFFFFFFFFFFFFFF) and `MHD_create_response_from_fd()` is invoked. To avoid such nonsense, we test the path now and allow regular files only. References: etr#255

LeSpocky · 2022-02-01T20:51:34Z

Those unit test are very repetitive. Maybe that can be improved later?

etr · 2022-02-01T22:36:40Z

Those unit test are very repetitive. Maybe that can be improved later?

Yeah, I think it would be worth keeping a change to those in an isolated change.

src/httpserver/file_response.hpp

etr

Can we also add information about the method new failing path in the README file?

This documents a possible pitfall for users when passing filename of not existing files. References: etr#255

The constructor of class `httpserver::file_response` can be called with a `filename` pointing to a directory instead of a regular file. The webserver should fail predictably in that case. References: etr#255

LeSpocky · 2022-02-02T20:26:38Z

I updated the API doc to the constructor of the file_response class as suggested. However on that other remark of yours …

Can we also add information about the method new failing path in the README file?

Not sure in what direction that should go? There is this section Building responses to requests which describes the different flavors of http_response classes. The improved failure handling was in the get_raw_response() method and over where it is called in finalize_answer(). Instead of crashing or sending back potentially nonsense responses, libhttpserver returns 500 now, and explicitly expects the user to make sure a file used for the file_response must exist and be a regular file. It was a good idea to check that on user side before already. What do you have in mind should be added to the README and where? 🤔

etr · 2022-02-02T20:52:02Z

In the section called "Building responses to requests" there is piece dedicated to the file_response.

file_response(const std::string& filename, int response_code = 200, const std::string& content_type = "text/plain"): Uses the filename passed in construction as pointer to a file on disk. The body of the HTTP response will be set using the content of the file. The other two optional parameters are the response_code and the content_type. You can find constant definition for the various response codes within the http_utils library file.

Don't need to make it too complicated. I'd just change it as below (the diff is in bold):

file_response(const std::string& filename, int response_code = 200, const std::string& content_type = "text/plain"): Uses the filename passed in construction as pointer to a file on disk. The body of the HTTP response will be set using the content of the file. The file must be a regular file and exist on disk. Otherwise libhttpserver will return an error 500 (Internal Server Error). The other two optional parameters are the response_code and the content_type. You can find constant definition for the various response codes within the http_utils library file.

Suggested-by: Sebastiano Merlino <sebastiano@hey.com> References: etr#255

etr · 2022-02-03T16:51:34Z

Thanks! This was a great piece of improvement for the library.

LeSpocky mentioned this pull request Jan 27, 2022

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

Closed

1 task

LeSpocky and others added 3 commits January 29, 2022 11:58

test: Add unit test for missing file response

103aedb

The constructor of class `httpserver::file_response` can be called with a `filename` pointing into the void, to a file which does not actually exist. The webserver should fail predictably in that case. References: etr#255

LeSpocky force-pushed the file-response branch from ddb09d7 to 103aedb Compare January 29, 2022 11:04

LeSpocky commented Jan 29, 2022

View reviewed changes

LeSpocky marked this pull request as ready for review January 30, 2022 11:04

LeSpocky marked this pull request as draft January 31, 2022 14:35

LeSpocky force-pushed the file-response branch from 61536f8 to 3089c89 Compare January 31, 2022 17:05

LeSpocky force-pushed the file-response branch from 3089c89 to 123d5db Compare January 31, 2022 17:16

LeSpocky marked this pull request as ready for review February 1, 2022 21:07

etr reviewed Feb 2, 2022

View reviewed changes

src/httpserver/file_response.hpp Outdated Show resolved Hide resolved

etr reviewed Feb 2, 2022

View reviewed changes

src/httpserver/file_response.hpp Outdated Show resolved Hide resolved

etr reviewed Feb 2, 2022

View reviewed changes

LeSpocky added 2 commits February 2, 2022 21:04

file_response: Add API doc to constructor

3ff2047

This documents a possible pitfall for users when passing filename of not existing files. References: etr#255

test: Add unit test for file_response pointing to directory

81bcc4d

The constructor of class `httpserver::file_response` can be called with a `filename` pointing to a directory instead of a regular file. The webserver should fail predictably in that case. References: etr#255

LeSpocky force-pushed the file-response branch from ce5960d to 81bcc4d Compare February 2, 2022 20:15

readme: Document requirements and behaviour of file_response

41d9eeb

Suggested-by: Sebastiano Merlino <sebastiano@hey.com> References: etr#255

etr merged commit 5cb0be1 into etr:master Feb 3, 2022

LeSpocky deleted the file-response branch February 3, 2022 17:16

File response #256

File response #256

Uh oh!

Conversation

LeSpocky commented Jan 27, 2022

Requirements for Contributing a Bug Fix

Identify the Bug

Description of the Change

Alternate Designs

Possible Drawbacks

Verification Process

Release Notes

Uh oh!

LeSpocky commented Jan 28, 2022

Uh oh!

LeSpocky commented Jan 29, 2022

Uh oh!

LeSpocky Jan 29, 2022

Choose a reason for hiding this comment

Uh oh!

LeSpocky commented Jan 31, 2022

Uh oh!

etr commented Jan 31, 2022

Uh oh!

LeSpocky commented Feb 1, 2022

Uh oh!

etr commented Feb 1, 2022

Uh oh!

Uh oh!

Uh oh!

etr left a comment

Choose a reason for hiding this comment

Uh oh!

LeSpocky commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etr commented Feb 2, 2022

Uh oh!

etr commented Feb 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LeSpocky commented Feb 2, 2022 •

edited

Loading