Bug 244123 - libfetch: memory leak when processing multiple HTTP location response headers
Summary: libfetch: memory leak when processing multiple HTTP location response headers
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-bugs (Nobody)
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2020-02-14 14:11 UTC by Hans Christian Woithe
Modified: 2020-02-14 22:54 UTC (History)
0 users

See Also:


Attachments
patch to libfetch (355 bytes, patch)
2020-02-14 14:11 UTC, Hans Christian Woithe
no flags Details | Diff
Simple Python server useful for testing (402 bytes, text/plain)
2020-02-14 14:13 UTC, Hans Christian Woithe
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Christian Woithe 2020-02-14 14:11:18 UTC
Created attachment 211646 [details]
patch to libfetch

fetchMakeURL() or fetchParseURL() are used to create a new url struct when processing a location header.  In the event that the HTTP response contains multiple location headers, the previously allocated url is freed using free() instead of fetchFreeURL().  This currently prevents the struct's "doc" member from being freed.

Please find attached a patch to use fetchFreeURL().
Comment 1 Hans Christian Woithe 2020-02-14 14:13:24 UTC
Created attachment 211647 [details]
Simple Python server useful for testing

Please find attached a simple Python server useful for testing the scenario described in the bug report.
Comment 2 Conrad Meyer freebsd_committer freebsd_triage 2020-02-14 17:42:16 UTC
Your fix looks correct to me.

Technically this is a non-compliant server, right?  IIRC, modern HTTP does not allow duplicate instances of headers.  So it would also be valid for libfetch to just reject the connection when encountering the second Location.  No?

This case is only encountered when one HTTP response contains two location headers and not by, e.g., 302 / Location: foo => 302 / Location: bar => ...
Comment 3 Hans Christian Woithe 2020-02-14 22:54:11 UTC
Duplicate headers can be allowed in certain situations.  I tend to agree that the location header probably doesn't qualify.

https://tools.ietf.org/html/rfc7230#section-3.2.2

A generic fix would involve a larger change on how headers are processed.  Such a change should detect all duplicate headers that aren't allowed and return an appropriate error to the user.  That could eliminate the need for the code under scrutiny.