For MSVC use alloca() instead of VLA

sphincs / sphincsplus

The SPHINCS+ reference code, accompanying the submission to NIST's Post-Quantum Cryptography project

https://sphincs.org

Other

131 stars 44 forks source link

For MSVC use alloca() instead of VLA #26

Closed bwesterb closed 2 years ago

bwesterb commented 2 years ago

Closes #20

@thomwiggers

thomwiggers commented 2 years ago

Was this tested? AFAICT MSVC doesn't have alloca, but it does have _alloca (deprecated) in malloc.h (for maximum confusion).

sfluhrer commented 2 years ago

I'm not certain that allocating variable length automatic arrays (either with VLAs or using alloca) is the Right Thing. Two examples:

haraka_S_absorb - the rate is always 32 - why do we need a variable length array?
thash (the robust versions) - currently, we allocate the full array, generate the full mask, and xor it in. An alternative approach would be generate the mask one block at a time, and process it incrementally - see https://github.com/sphincs/parallel-sphincsplus/blob/main/shake256_robust.cpp for an example of that being done.

thomwiggers commented 2 years ago

Even though this perhaps isn't at all the best way to solve this, it would be great if we can actually merge some form of this PR so that we can proceed with updating PQClean (and OQS downstream).

bwesterb commented 2 years ago

@sfluhrer I want to propose to merge a rebased version of this patch together with Thom's suggestion for now. Then, later, we can get rid of the variable sized arrays alltogether.

bwesterb commented 2 years ago

@thomwiggers Rebased. If builds pass, please give it a double-check.

thomwiggers commented 2 years ago

Seems to compile just fine using MSVC. There's some more work needed before this repo will produce useful binaries (mostly because the test binaries here use OpenSSL and the random number generator isn't set up for Windows) -- but I'm not getting VLA problems here.