Re: [PATCH v2] mceliece6688128f: fix stack overflow crash on win64/wine

On 28/09/2025 04:33, Jacob Bachmeyer via Gcrypt-devel wrote:
> On 9/27/25 02:02, Jussi Kivilinna wrote:
>> * cipher/mceliece6688128f.c (pk_gen): Remove 'mat' array allocation and
>> rename function to ...
>> (pk_gen_mat): ... this.
>> (pk_gen): New wrapper for 'pk_gen_mat' with dynamic allocation of 'mat'
>> array.
>> -- 
>>
>> Huge array allocations from stack are not always guaranteed to work on
>> every target platform, so avoid allocating multi-megabyte 'mat' array
>> from stack.
>>
>> v2: zero 'mat' array after use.
>>
>> Signed-off-by: Jussi Kivilinna <[email protected]>
>> ---
>>   cipher/mceliece6688128f.c | 23 +++++++++++++++++------
>>   1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/cipher/mceliece6688128f.c b/cipher/mceliece6688128f.c
>> index ca1952b5..bdc7f265 100644
>> --- a/cipher/mceliece6688128f.c
>> +++ b/cipher/mceliece6688128f.c
>> [...]
>> @@ -3460,6 +3459,18 @@ static int pk_gen(unsigned char * pk, const unsigned char * irr, uint32_t * perm
>>   }
>> +static int pk_gen(unsigned char * pk, const unsigned char * irr, uint32_t * perm, int16_t * pi, uint64_t * pivots)
>> +{
>> +    /* Allocate large array from heap to avoid stack overflow crash on Win32/Wine. */
>> +    unsigned int sizeof_mat = sizeof(uint64_t) * PK_NROWS * nblocks_H;
> 
> Would this make sense as "const unsigned int sizeof_mat" to enable better compiler optimizations/inlining since PK_NROWS and nblocks_H seem to be constants?  Or is GCC able to deduce that sizeof_mat is only assigned once?

With GCC -O0, "unsigned int sizeof_mat" and "const unsigned int sizeof_mat" generate the same code, value is assigned to stack memory. With -O1 and above, compiler deduces that this is constant variable and optimizes as it sees best fit. 'const' on stack variables only really affects syntax checks and gives compile-time error when trying reassigning to that variable.

> 
> Also, would "size_t" be a more appropriate base type than unsigned int, yielding "const size_t sizeof_mat = [...]"?

I saw that "unsigned int" is large enough for that value, but I'll change type to "size_t" for consistency.

-Jussi

_______________________________________________
Gcrypt-devel mailing list
[email protected]
https://lists.gnupg.org/mailman/listinfo/gcrypt-devel