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 when not in --release #60

Open
henke443 opened this issue May 3, 2022 · 16 comments
Open

Stack overflow when not in --release #60

henke443 opened this issue May 3, 2022 · 16 comments

Comments

@henke443
Copy link

henke443 commented May 3, 2022

I just started using this library. I'm on windows 10 x64 if that makes a difference.
When building with --release it compiles correctly and I get the right results, when I omit it things fail however:

thread 'main' has overflowed its stack
error: process didn't exit successfully: target\debug\npcoin.exe (exit code: 0xc00000fd, STATUS_STACK_OVERFLOW)

Another small thing is that I failed to make the examples in the README run before I looked through the code and found out that I have to specify a 32 byte seed with 64 hex characters.

@aewag
Copy link
Member

aewag commented May 4, 2022

I just started using this library. I'm on windows 10 x64 if that makes a difference. When building with --release it compiles correctly and I get the right results, when I omit it things fail however:

thread 'main' has overflowed its stack error: process didn't exit successfully: target\debug\npcoin.exe (exit code: 0xc00000fd, STATUS_STACK_OVERFLOW)

I dont experience the issue on my machinge, but:
Stack usage of the implementation can be bigger than expected, as we do not dynamically allocate any memory.
You can give it a shot and increase the RUST_MIN_STACK.

Another small thing is that I failed to make the examples in the README run before I looked through the code and found out that I have to specify a 32 byte seed with 64 hex characters.

Yeah, true lms-demo could be more more verbose. I'll add more output to it.

@henke443
Copy link
Author

henke443 commented May 5, 2022

Thanks a lot, great code also keep it up 🙌
My initial report was pretty bad so here's some more information:
The specific code which fails is:

  let (signing_key, verifying_key) = keygen(&[parameter], &seed, None)
  .unwrap_or_else(|_| panic!("Could not generate keys"));

When running the example without the --release flag everything actually works. So it only seems to happen when adding

[dependencies]
    hbs-lms = "*"

@aewag
Copy link
Member

aewag commented May 6, 2022

Just for clarification:
Does our example lms-demo fail on your machine or does your code fail after adding hbs-lms as dependency and trying to generate a key?

@henke443
Copy link
Author

henke443 commented May 6, 2022

Only after adding hbs-lms as a dependency

@aewag
Copy link
Member

aewag commented May 6, 2022

Can you give it a try and add a config.toml to your project and set the RUST_MIN_STACK to "8000000"?

This as well add for the lms-demo example.

@henke443
Copy link
Author

henke443 commented May 6, 2022

I've created a minimal example that errors in debug but not release for me, can you see if it works for you?
https://github.com/henke443/hbs-stack-overflow

I've also forked the repository and tried to change ArrayVec to TinyVec but it doesn't seem to solve the issue. Additionally for me, when running cargo test --doc the second test fails. This is the only test that fails.

test src\hss\mod.rs - hss::hss_keygen (line 258) ... ok
test src\lib.rs - (line 14) ... FAILED

failures:

---- src\lib.rs - (line 14) stdout ----
Test executable failed (exit code -1073741571).

stderr:

thread 'main' has overflowed its stack



failures:
    src\lib.rs - (line 14)```

@aewag
Copy link
Member

aewag commented May 6, 2022

I've created a minimal example that errors in debug but not release for me, can you see if it works for you? https://github.com/henke443/hbs-stack-overflow

Nice, thanks for the minimal example!

I've also forked the repository and tried to change ArrayVec to TinyVec but it doesn't seem to solve the issue. Additionally for me, when running cargo test --doc the second test fails. This is the only test that fails.

test src\hss\mod.rs - hss::hss_keygen (line 258) ... ok
test src\lib.rs - (line 14) ... FAILED

failures:

---- src\lib.rs - (line 14) stdout ----
Test executable failed (exit code -1073741571).

stderr:

thread 'main' has overflowed its stack



failures:
    src\lib.rs - (line 14)```

Unfortunately, on my machine I cannot recreate this error. Neither the minimal nor the cargo test --doc triggers a StackOverflow error.

I have never tested the functionality on a windows machine. Maybe it stems from this. But I am not sure at this point.

Which rustc version are you using?

@henke443
Copy link
Author

henke443 commented May 6, 2022

I'm using rustc 1.60.0 (7737e0b5c 2022-04-04).

Yes that's probably it then. Since it works for Linux and is only a problem in Debug this issue is not deal-breaking, so I'll close it. Thanks for investigating!

@henke443 henke443 closed this as completed May 6, 2022
@henke443
Copy link
Author

henke443 commented May 7, 2022

Actually, my TinyVec change did solve the issue! My project now works as expected, however, the second doc test is still failing. Additionally, because src/hss/aux.rs is named "aux.rs", Windows prevents it from being created which is an issue when trying to clone the project. (https://stackoverflow.com/a/67226358/13819662 Very dumb, Linux is superior). I can create a PR for my changes, I'm afraid it will be quite large and maybe you can see some optimizations for it.

@henke443 henke443 reopened this May 7, 2022
@aewag
Copy link
Member

aewag commented May 8, 2022

Ah, yes you activated the alloc feature and enabled the use of it. I am in favor to enhance this library with this feature, but the ability to not require alloc is for us important as we would like to enable an embedded use case, where a heap is most of the times not available.

If you could add an alloc feature to this crate and depending if it is available to use TinyVec and if not use ArrayVec would be cool! Maybe you could move this logic into a separate file under util and only import the chosen vector backend from there.

For the point with renaming: As it seems like a generic issue, it makes sense to take into account.

@henke443
Copy link
Author

henke443 commented May 8, 2022

Okay I will attempt to do that :) I will also make multiple PRs for the different features. Will be done in a week I think.

@aewag
Copy link
Member

aewag commented May 9, 2022

Cool, thank you. 👍

@cshroffgithub
Copy link

newbie question. How/where do you specify seed for example demo ?

@aewag
Copy link
Member

aewag commented Dec 5, 2022

newbie question. How/where do you specify seed for example demo ?

It is added as argument to the keygen cmd (I just realized that the readme is out-of-date and the example needs some polish, I moved that into a separate issue #83)

cargo run --release --example lms-demo -- genkey mykey 10/2 --seed 0123456701234567012345670123456701234567012345670123456701234567

@aewag
Copy link
Member

aewag commented Dec 5, 2022

@henke443 Are you still interested to work on a solution for the issue of memory usage under Windows?

@cshroffgithub
Copy link

thank you @aewag. BTW, i am glad to see that this was published. It was great to collaborate with your team.

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