Fix possible leaks in SAPI.c sapi_read_post_data

[hwde]

• content_type_dup may leak on further post request
• local content_type isn’t free’ed on unsupported content-type

[Girgias]

Would it be possible to write a test case for this?

[cmb69]

Thank you! Looks good on quick glance, but a test would be nice (might not be worth the trouble, though).

Anyhow, I think we should convert sapi_request_info.content_type and .content_type_dup to zend_string at some point in time (it would be a BC break), so we could avoid the actual duplication.

[Girgias]

I think the SOAP test failure is related to this because it uses the CLI sapi?

[arnaud-lb]

I'm not sure that we are supposed to free SG(request_info).content_type_dup in this case. If I understand this code correctly, sapi_read_post_data() is supposed to initialize SG(request_info).content_type_dup and ignore any previous value. A non-NULL SG(request_info).content_type_dup would be a freed pointer from a previous request.

[hwde]

I see in sapi_deactivate() there is a efree(SG(request_info).content_type_dup); for non null content_type_dup without set it to NULL afterwards. Overall, an unfortunate coding style.
If NULL / not NULLs are used consistently with free() it should be easier to see through overall.

[bukka]

So I had a closer look and the sapi_deactivate is not the only place that frees SG(request_info).content_type_dup. I see also sapi_handle_post which is called by treat_data hook which I assume will be called later. It also sets it to NULL so it should never happen that SG(request_info).content_type_dup is not NULL here unless I missed something..? I think it doesn't really do any harm to add it but I don't see it as a fix in such case.

[bukka]

Seems reasonable to me. Not sure if the test failure is related as it fails on connection.

[devnexen]

@hwde thanks for your contributions. However, for a bug fix it s better to target the lowest major branch where the said patch apply. Thankfully your previous fix and this one apply just fine to PHP-8.0 ;-)

[devnexen]

Looks fine by me too, but feeling a bit "nervous" accepting it tough. @hwde just trying to decrease the chance of your fix being reverted afterwards :-)

[bukka]

I think this is actually the only part that fixes a real leak.

[bukka]

It might be a good idea to try to write a test for this and separate from the previous checks

[github-actions]

There has not been any recent activity in this PR. It will automatically be closed in 7 days if no further action is taken.

[bukka]

I looked properly again into this to be sure we didn't leave some memory leak around. After checking again the code and doing a bit of debugging as well, I noticed that there is actually no memory leak in reality.

As mentioned SG(request_info).content_type_dup is always uninitialized at this stage (request startup). It means it's either fresh (first request) or freed in the previous request sapi_deactivate_module. So no need to do any checking of existing value. In fact it would actually do double free as it stands because sapi_deactivate_module doesn't set NULL to the freed value so there is probably still previously freed address...

In terms of content_length, it's only leaking in the part that is not actually reachable by the look of it. It's because the default_post_reader is set by php_startup_sapi_content_types which is called during the startup. So unless I missed anything, it should always be set.

For better clarity I added efree for the content_type and also marked that block as UNEXPECTED in the linked commit which also adds few more comments for further clarification.