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

Stack overflow from jas_image_writecmpt() #256

Closed
dgr37 opened this issue Dec 11, 2020 · 4 comments · Fixed by #257
Closed

Stack overflow from jas_image_writecmpt() #256

dgr37 opened this issue Dec 11, 2020 · 4 comments · Fixed by #257

Comments

@dgr37
Copy link

dgr37 commented Dec 11, 2020

With jasper 2.0.23 I am having major problems with decompression of medium-sized jpeg2000 files, resulting in a stack overflow. These problems did not occur in jasper 2.0.19. For example, if I take the attached .jp2 file and do a decompression such as

jasper.exe --input "BACanonEos400D 003.jp2" --output test.jp2 --output-format jp2

stack memory is already exhausted and jasper crashes. Compiler is MSVC.

I noticed that the stack overflow arises from jas_image.c:534, jas_image_writecmpt(), where _alloca() is called explicitly to allocate memory on stack. Replacing this by a heap allocation, e.g. with jas_alloc2(), solves the problem. I attached a patch for this.

Is there a reason why stack is preferable here?

Thank you!

jas_image_writecmpt.zip

@jubalh
Copy link
Member

jubalh commented Dec 11, 2020

Thank your for the report and investigation!

These problems did not occur in jasper 2.0.19

The change was done in 5017d3a so is available since jasper 2.0.20

I noticed that the stack overflow arises from jas_image.c:534, jas_image_writecmpt(), where _alloca() is called explicitly to allocate memory on stack. Replacing this by a heap allocation, e.g. with jas_alloc2(), solves the problem. I attached a patch for this.
Is there a reason why stack is preferable here?

@MaxKellermann probably you know more about this.

@MaxKellermann
Copy link
Contributor

The problem is that alloca() is called for every iteration. That needs to be moved out of the loop.
The problem doesn't exist on C99 compliant systems because that stack allocation is only done once with C99.
Having the array declaration inside the loop may give the compiler more opportunities for optimization, because we tell him that he doesn't need to preserve the contents.

MaxKellermann added a commit to MaxKellermann/jasper that referenced this issue Dec 11, 2020
Fixes stack overflow bug on Windows, where variable-length arrays are
not available.  Unlike VLAs, _alloca() calls accumulate, and calling
it in every loop iteration is certainly a bad idea.

Regression from commit 5017d3a

Closes jasper-software#256
@jubalh
Copy link
Member

jubalh commented Dec 11, 2020

Thanks @MaxKellermann !

@dgr37 please give feedback here once you have tested.

@dgr37
Copy link
Author

dgr37 commented Dec 14, 2020

Thank you @jubalh and @MaxKellermann !

The change works perfectly for me.

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 a pull request may close this issue.

3 participants