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

LZOxide too large for Windows #21

Closed
mvdnes opened this issue Jan 27, 2018 · 5 comments
Closed

LZOxide too large for Windows #21

mvdnes opened this issue Jan 27, 2018 · 5 comments

Comments

@mvdnes
Copy link

mvdnes commented Jan 27, 2018

On Windows, the following code produces a stack overflow:

extern crate miniz_oxide;

use miniz_oxide::deflate::core::CompressorOxide;

struct Foo {
    bar: CompressorOxide,
}

fn main() {
    let foo = Foo { bar: CompressorOxide::new(0), };
}

Specifically, I get this error: thread 'main' has overflowed its stack.
This does not happen in release mode, however.

I think the structure LZOxide may be too large, as the overflow happens in it's new() funtion.

Perhaps it is a good idea to allocate this data on the heap?

@oyvindln
Copy link
Collaborator

Yea, there are some buffers which take up a fair bit of space. I believe the api used through flate2 actually does allocate on the heap, however without optimizations compiled code ends up with a lot of redundant stack copies, so it's easy to get overflows. Even with optimizations the current rustc/llvm does a bad job at getting rid of stack copies of more complex objects. This is a bit frustrating when dealing with statically sized buffers since if you use vec you lose the static length info which helps the compiler avoid bounds checks, but if you use boxed arrays you risk blowing the stack.

miniz_oxide is originally a port of miniz, which is written in C, and thus designed around the structures be allocated with malloc or similar, so having everything in one big struct makes sense rather than the internal buffers being analogous to Vec in rust (and also promises not doing any dynamic memory allocation internally). For rust this is not really ideal though, due to the mentioned issues, so trying to put stuff on the heap while trying to avoid stack copies seems reasonable.

@mvdnes
Copy link
Author

mvdnes commented Jun 22, 2018

In the latest master, this issue does not seem to be a problem any more. At least on Windows 10 x64 using

rustc 1.27.0 (3eda71b00 2018-06-19)
binary: rustc
commit-hash: 3eda71b00ad48d7bf4eef4c443e7f611fd061418
commit-date: 2018-06-19
host: x86_64-pc-windows-msvc
release: 1.27.0
LLVM version: 6.0

I get no more stack overflows using this rather in contrast to 0.1.2.

Could you publish a new version to crates.io?

@oyvindln
Copy link
Collaborator

Yeah, moved some stuff to the heap in f907ff7.
There are some other minor fixes in master too, so hopefully @Frommi can upload a new version soon.

@Frommi
Copy link
Owner

Frommi commented Jun 23, 2018

0.1.3 uploaded.

@mvdnes
Copy link
Author

mvdnes commented Jun 29, 2018

The testcase I provided works now, so I think this resolves it. Thanks!

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

No branches or pull requests

3 participants