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

Don't put an absurd amount of data onto the stack in some configs #692

Closed
gmaxwell opened this issue Nov 9, 2019 · 3 comments · Fixed by #988
Closed

Don't put an absurd amount of data onto the stack in some configs #692

gmaxwell opened this issue Nov 9, 2019 · 3 comments · Fixed by #988

Comments

@gmaxwell
Copy link
Contributor

gmaxwell commented Nov 9, 2019

PR #337 adds a --with-ecmult-gen-precision=8 which puts >672kb onto the stack during gen_context or at runtime in signing context creation if --disable-ecmult-static-precomputation is also used.

Default stacks being 8MB is mostly a 64-bit linux distroism.

I expect this gen_context and context create with no statick precomp now crashes with that config on some systems. I believe the default stack on Windows is 1MB, linux kernel code stack size is 8kb, 32-bit linux default stack size is 2MB. Lots of other systems have a modest default stack. IIRC firefox multimedia threads have a 128kb stack.

And, of course, whatever stack the library uses is added onto the caller, so it pays to be pretty conservative.

The commit message seems to indicate that valgrind was telling you that the stack was out of control. You probably don't want to ignore that-- the valgrind default setting is the amount needed for 32-bit linux to not barf, some other systems as I mention are even more constrained.

For normal usage this library shouldn't put more than a few kilobytes onto the stack. There is an issue which specifies making the stack usage explicit (#188). I suppose allowances could be made for weird configs like --disable-ecmult-static-precomputation + --with-ecmult-gen-precision=8, but even there doing well over 100kb will probably start crashing real programs on real systems.

Tests could probably get away with a fair bit more, since they control the entire calling stack and aren't going to cause mysterious runtime crashes-- but it would still be nice to be able to run them directly on the sort of low memory devices that can run the library.

@apoelstra
Copy link
Contributor

#546 would fix this.

@laanwj
Copy link
Member

laanwj commented Nov 10, 2019

Ah yes, I also stumbled into this one once. I remember some tweaking of the parameters solved it, but yes, I agree being able to put half a MB of data on the stack is not a good default assumption.

You might want to compile wth -Wframe-larger-than=... -Werror=frame-larger-than (r something like that) in the CI to detect these and fail on it.

@gmaxwell
Copy link
Contributor Author

gmaxwell commented Nov 10, 2019

Does anyone have a usecase for gen-precision=8 or was it just added for completionism?

If no one does, it should probably be dropped, @laanwj's suggested warnings (Werror in CI only) added to clamp it to just above the peak usage with 4.

Then hopefully 546 is eventually included, considering it looks like it gives 8's performance with less memory than 4. :)

It would be nice to guarantee in the standard config that it doesn't use more than 4kb or so at runtime or at least so for non-batch verification, which I suspect is already achieved.

@gmaxwell gmaxwell changed the title Don't put an absurd amount of data onto the stack Don't put an absurd amount of data onto the stack in some configs Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants