Skip to content
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.

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

Plug memory leaks #2183

Merged
merged 3 commits into from
Jun 2, 2024
Merged

Plug memory leaks #2183

merged 3 commits into from
Jun 2, 2024

Conversation

stoeckmann
Copy link
Contributor

The parsers for 7zip and xar contain memory leaks. Also the testsuite 7zip leaks memory. Plug them.

Noticed while running test suite with address sanitizer on Linux.

If zstd support exists, zstd_dstream has to be freed like other
streams.
If parsing fails, free the specific XML parser.
Free archive reader before allocating a new one.
@@ -3316,8 +3319,10 @@ expat_read_toc(struct archive_read *a)

d = NULL;
r = rd_contents(a, &d, &outbytes, &used, xar->toc_remaining);
if (r != ARCHIVE_OK)
if (r != ARCHIVE_OK) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this. This is certainly right if rd_contents returns ARCHIVE_FATAL, but if it returns something less serious, should we really release the parser here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In line 3340 it's released in every case as well. The variable parser is local and allocated in line 3302. I'd assume that it cannot be used outside and should be released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least that's how I understand the code. I don't mind being corrected. In fact, it would be better to get this right at the first attempt.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're right. Since parser is local, we need to be certain to clean it up here. Good catch!

@@ -3316,8 +3319,10 @@ expat_read_toc(struct archive_read *a)

d = NULL;
r = rd_contents(a, &d, &outbytes, &used, xar->toc_remaining);
if (r != ARCHIVE_OK)
if (r != ARCHIVE_OK) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're right. Since parser is local, we need to be certain to clean it up here. Good catch!

@kientzle kientzle merged commit edbb1fd into libarchive:master Jun 2, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants