Fix UB by avoiding duplicate httpd_resp_send_x

[7FM]

parse_get already cleans buf if something went wrong. Therefore the concatenated if conditions containing parse_get needs to be split to avoid UB in some cases which might cause a crash.

[me-no-dev]

What is UB and sorry but I did not get your explanation? In what case this code will break?

[7FM]

UB stands for Undefined Behaviour meaning it is not specified what will happen but most likely it will be a Segmentation fault from my experience. The code will likely fail if httpd_req_get_url_query_str fails inside parse_get: if so free(buf) will be called which is good because it is not possible to determine from outside the function if buf needs to be freed (malloc might have also failed or no GET arguments were given=> no memory was allocated). Besides httpd_resp_send_404(req); will be called as well and ESP_FAIL will be returned in such a case. If parse_get is now used like that:

    if (parse_get(req, &buf) != ESP_OK ||
        httpd_query_key_value(buf, "reg", _reg, sizeof(_reg)) != ESP_OK ||
        httpd_query_key_value(buf, "mask", _mask, sizeof(_mask)) != ESP_OK ||
        httpd_query_key_value(buf, "val", _val, sizeof(_val)) != ESP_OK) {
        free(buf);
        httpd_resp_send_404(req);
        return ESP_FAIL;
    }

The condition would be true in that case causing another call of free(buf) which is Undefined Behaviour because that memory location was already freed. Even:

    if (parse_get(req, &buf) != ESP_OK) {
        free(buf);
        httpd_resp_send_404(req);
        return ESP_FAIL;
    }

causes UB.

[me-no-dev]

ok, I checked the code. the buffer that is freed inside parse_get is not the same buffer. Proper fix is to remove the httpd_resp_send_x lines from parse_get. That is the only change needed.

[me-no-dev]

well maybe I said this wrong. if parse_get returns error, buf will be NULL, so there is no problem to call free on NULL. if anything else fails, buf will not be NULL (set by parse_get) so then free will actually free the buffer. Again, removinghttpd_resp_send_x will fix the only probably fail (UB)

[7FM]

well maybe I said this wrong. if parse_get returns error, buf will be NULL, so there is no problem to call free on NULL. if anything else fails, buf will not be NULL (set by parse_get) so then free will actually free the buffer.

Oh I see, my fault somehow I got in mind that buf will always be set xD Sorry for confusion!

Proper fix is to remove the httpd_resp_send_x lines from parse_get. That is the only change needed.

Hm not sure about that, if you would remove those lines then there would be no more differentiation between allocation error (return code 500) or invalid request (return code 404). WIth that in mind my changes would still fit.

[7FM]

I changed commit message and pull request name accordingly