Use variable-length array instead of malloc/free.

[ryancdotorg]

The only use of malloc/free in tomfastmath is in src/numtheory/fp_prime_random_ex.c, and the size required can be calculated from the function arguments, so we can just use a variable-length automatic array instead.

Variable-length automatic arrays are a C99 feature, and TFM already doesn’t compile as C90, so we might as well take advantage of it.

This is a smaller part of a patchset to allow tomsfastmath to be compiled without stdlib, e.g. for freestanding webassembly.

[levitte]

There's also #4, which while being quite old at this point, also does away with memory allocation by having a fixed size buffer.

[ryancdotorg]

I'd be happy to have that merged instead, though I think the variable length array is more elegant.

…

On September 16, 2024 5:59:02 AM GMT+01:00, Richard Levitte ***@***.***> wrote: There's also #4, which while being quite old at this point, also does away with memory allocation by having a fixed size buffer.

[levitte]

In the end, nothing stops both from being merged 😉

[sjaeckel]

I agree that the dynamic allocation does not really make sense here.

I don't like VLA's, so I'd propose to use a stack based buffer of maximum size instead.

[sjaeckel]

Suggested change

	unsigned char tmp[(size>>3)+(size&7?1:0)];
	unsigned char tmp[sizeof(a->dp)];

Why not simply allocate a statically sized stack array of max size?

This would also require something like the following, after calculating bsize.

if (bsize > sizeof(tmp))
   return MP_VAL;

[levitte]

Would the use of VLAs diminish the risk of stack overflows?

Ref: libtom/libtomcrypt#461 (comment)

[sjaeckel]

Would the use of VLAs diminish the risk of stack overflows?

FMU the use of VLAs as done here increases the risk of a stack overflow, since there's no upper boundary of size implemented.

Calling this version of the API like this: fp_prime_random_ex(&foo, bar, 8192*1024*8, baz, qux, quux) would already overflow the stack of most linux systems. Does it make sense? No. Is it possible and therefore a potential problem? IMO yes.

The upper boundary of size is effectively the max size of an fp_int, so supporting bigger sizes does not make sense. Even if this would be fixed with a check I'm not sure if there would be any advantage of a VLA approach here over a fixed size array.

With a VLA here the stack usage would indeed be minimized in all cases where size is smaller than the max size, but that's not something that is usually done when modelling this problem, right? In that model one usually expects that numbers are huge and it still shouldn't fail unexpectedly, e.g. by overflowing the stack.

[sjaeckel]

Ref: libtom/libtomcrypt#461 (comment)

TBH I never checked the max. stack depth of tfm in detail and I just thought that this could become a problem if you increase the fp_int size too much. Now that I did the numbers that's maybe also not really the case...

To be able to work with RSA16384 keys one has to set #define FP_MAX_SIZE ((16384*2)+(8*DIGIT_BIT)), which increases the stack usage of each fp_int to more than 4KiB. This would allow >2k of those fp_int's with the default stack size, which can't be reached by calling TFM API's alone. TFM has no recursive functions AFAICT and what I saw when checking the more complex APIs was far below this number.

One thing I'm not sure of is if (8*DIGIT_BIT) is enough for such big sizes. IIRC that's some scratch space for comba or montgomery and I'm not deep enough in the math theory to tell if that number should grow with the total size or if that's just some scratch area that can be fixed size. The documentation says that it should be 4 digits, but the implementation was changed in tfm 0.06 to 8 digits... Maybe someone else can tell us more on that? I'm preventive mentioning @czurnieden here, in case nobody else pops up :)

[levitte]

Okie, I understand re VLA. For the rest, I'll try to make time to look at that scratch space to see what's what

[levitte]

Considering all the commentary here, I rebooted #4 → #39. Please have a look at that. @ryancdotorg, I would your commentary there.

[ryancdotorg]

I'll try to understand it on Sunday, but TBH it's been a while since I touched it, so I'm not sure how much help I can provide. I'm happy with any solution that avoids malloc.

[levitte]

I'll try to understand it on Sunday, but TBH it's been a while since I touched it, so I'm not sure how much help I can provide. I'm happy with any solution that avoids malloc.

@sjaeckel merged #39 now, so you at least got rid of malloc