-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Comments
#546 would fix this. |
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 |
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. |
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.
The text was updated successfully, but these errors were encountered: