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

Compressor::feed may wrongly allocate a lot of memory. #578

Closed
mgautierfr opened this issue Jun 24, 2021 · 2 comments · Fixed by #594
Closed

Compressor::feed may wrongly allocate a lot of memory. #578

mgautierfr opened this issue Jun 24, 2021 · 2 comments · Fixed by #594
Assignees
Labels
Milestone

Comments

@mgautierfr
Copy link
Collaborator

The commit a7201ce
wrongly try to allocate a bigger buffer for the output data (a7201ce#diff-f89546dd4147a65d6ae025e38c7d09ce535c435db7f27984dc5256653d5a5c65R224-R245)

Before we were allocating in case of BUFFER_ERROR when stream.avail_out == 0 now we allocate simply on BUFFER_ERROR.

This may be the cause of the failing creation of Gutenberg with a std::bad_alloc (https://farm.openzim.org/pipeline/bc15fab2d29dc287d8fdec06/debug) (to confirm)

@mgautierfr mgautierfr added the bug label Jun 24, 2021
@mgautierfr mgautierfr added this to the libzim 7.0.0 milestone Jun 24, 2021
@mgautierfr mgautierfr self-assigned this Jun 24, 2021
@mgautierfr mgautierfr changed the title Compressor::feed wrongly allocate a lot of memory. Compressor::feed may wrongly allocate a lot of memory. Jun 24, 2021
@kelson42
Copy link
Contributor

@mgautierfr I thought you have understood what is going on here, but I see not branch/PR related to id, even in draft!?

@kelson42
Copy link
Contributor

@mgautierfr What is the status here? We just have released version 1.1.7 of the Gutenberg scraper which introduce zstd conpression in place of lzma. It seems that for this scraper at least, we have/had a regression impacting the lzma compression. Considering that lzma is going to be deprecated at some point, I believe fixing a bug on lzma conpression is not a priority. The question left is: do we have an additional problem? A problem which impacts zstd as well? What is its exact impact?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants