[fetch] Fix memory leak inside saveResponseAndStatus in streaming mode

[inolen]

when streaming was enabled -

• onprogress allocated emscripten_fetch_t.data
• onload called saveResponseAndStatus
• xhr.response was NULL (due to streaming) which caused ptr to remain 0
• emscripten_fetch_t.data was uncondtionally overwritten by ptr, leaking what was allocated during onprogress

This change also wraps realloc in a guard to only call it if necessary (new buffer is larger than existing).

Not sure what's the best way to unobtrusively test this - is there some setting that enables some basic leak detections we could enable for an existing test that uses streaming (e.g. test_fetch_stream_async)?

[inolen]

The test there is weird - it's testing that data is NULL in onload which would indicate a leak, since data isn't free'd until emscripten_fetch_close is called.

Edit: to clarify, I think that assert() should be removed from the test, or else onprogress would need to become responsible for free'ing data before onload is called and before the user calls emscripten_fetch_close which seems like a strange behavior.

[sbc100]

It seems like the idea is that with streaming you get the bytes only in the progress events and not in the onsuccess event.

At least that has been the way the API has been since this test was added 10 years ago: 786f2fe

[inolen]

Yep, but the same structure (with the ->data pointer) is passed to both events. To keep the test as is, ->data would have to be free'd and NULL'd before calling onsuccess.

As I said in a previous reply, I think of ->data as the persistent buffer between events and ->numBytes as the amount of data in that buffer that's valid for the current event. ->numBytes being 0 is important, but ->data being required to be NULL in onload seems odd.

[sbc100]

The test there is weird - it's testing that data is NULL in onload which would indicate a leak, since data isn't free'd until emscripten_fetch_close is called.

Edit: to clarify, I think that assert() should be removed from the test, or else onprogress would need to become responsible for free'ing data before onload is called and before the user calls emscripten_fetch_close which seems like a strange behavior.

I think the idea is that with stream you get a series ->data chunks, each on a realloc of the previous. This memory is only then free'd in fetch_free I think? Where were you thinking it should be free'd?

[sbc100]

Since ptr is initialized to emscripten_fetch_t.data and then used in realloc and then re-assigned to emscripten_fetch_t.data what is basically happening is like writing this in C:

f->data = realloc(f->data, newsize);

In this case there is no leak, right? Only the new f->data need be freed.

[inolen]

Since ptr is initialized to emscripten_fetch_t.data and then used in realloc and then re-assigned to emscripten_fetch_t.data what is basically happening is like writing this in C:

f->data = realloc(f->data, newsize);

In this case there is no leak, right? Only the new f->data need be freed.

It's a bit hidden - xhr.response is NULL when streaming is used. In saveResponseAndStatus (called by onload), if response.xhr is NULL, the data pointer is set to NULL (at which point its leaked).

I think the idea is that with stream you get a series ->data chunks, each on a realloc of the previous. This memory is only then free'd in fetch_free I think? Where were you thinking it should be free'd?

Absolutely. However, I think of ->data more as a persistent backing buffer. It gets realloced throughout the fetch, and then finally free'd by emscripten_fetch_close. I don't think there's any need for ->data to be NULL going into the onload callback (as the test is testing). The only reason it's NULL right now is because it's been leaked 😆

[sbc100]

I see, so if we want to keep the current API (with data NULL in onload under streaming) would need to call free on ->data before setting it to null for the onload callback