Skip to content

Fix possible leaks in SAPI.c sapi_read_post_data #8800

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix possible leaks in SAPI.c sapi_read_post_data #8800

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion main/SAPI.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,15 +209,22 @@ static void sapi_read_post_data(void)
SG(request_info).post_entry = NULL;
if (!sapi_module.default_post_reader) {
/* no default reader ? */
SG(request_info).content_type_dup = NULL;
if (SG(request_info).content_type_dup) {
efree(SG(request_info).content_type_dup);
SG(request_info).content_type_dup = NULL;
Comment on lines +212 to +214
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@hwde hwde Jun 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
sapi_module.sapi_error(E_WARNING, "Unsupported content type: '%s'", content_type);
efree(content_type);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return;
}
}
if (oldchar) {
*(p-1) = oldchar;
}

if (SG(request_info).content_type_dup) {
efree(SG(request_info).content_type_dup);
}
SG(request_info).content_type_dup = content_type;

if(post_reader_func) {
Expand Down Expand Up @@ -466,6 +473,9 @@ SAPI_API void sapi_activate(void)
* depending on given content type */
sapi_read_post_data();
} else {
if (SG(request_info).content_type_dup) {
efree(SG(request_info).content_type_dup);
}
SG(request_info).content_type_dup = NULL;
}

Expand Down