Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. GnuPG>
  3. gcrypt
Uninteded Variable Length Array in ec-nist.c
gniibe at fsij
Sep 26, 2022, 5:54 PM
Post #1 of 5 (449 views)
Permalink
Hello,

While we allow use of some features of C99 for libgcrypt, we don't use
variable length array in the code.

Thus, I'm considering adding -Wvla option in configure.ac. In master,
I found use of variable length array in ec-nist.c. I attach the
warning message of compiler when it is compiled with -Wvla option.

I think that it's not intended. Unfortunately, even if we added const
qualifier to the variable 'wsize', (because what is needed here is a
constant expression), it is still considered as variable length array by
compilers.

I think that use of macro for the size is needed here, although it would
not look modern code.

==========================
gcc -DHAVE_CONFIG_H -I. -I../../../libgcrypt/mpi -I.. -I../src -I../../../libgcrypt/src -I/usr/local/include/x86_64-linux-gnu -g -O2 -fvisibility=hidden -fno-delete-null-pointer-checks -Wall -Wvla -MT ec-nist.lo -MD -MP -MF .deps/ec-nist.Tpo -c ../../../libgcrypt/mpi/ec-nist.c -fPIC -DPIC -o .libs/ec-nist.o
../../../libgcrypt/mpi/ec-nist.c: In function '_gcry_mpi_ec_nist192_mod':
../../../libgcrypt/mpi/ec-nist.c:98:3: warning: ISO C90 forbids variable length array 's' [-Wvla]
98 | mpi_limb64_t s[wsize + 1];
| ^~~~~~~~~~~~
../../../libgcrypt/mpi/ec-nist.c:99:3: warning: ISO C90 forbids variable length array 'o' [-Wvla]
99 | mpi_limb64_t o[wsize + 1];
| ^~~~~~~~~~~~
../../../libgcrypt/mpi/ec-nist.c: In function '_gcry_mpi_ec_nist224_mod':
../../../libgcrypt/mpi/ec-nist.c:191:3: warning: ISO C90 forbids variable length array 's' [-Wvla]
191 | mpi_limb64_t s[wsize];
| ^~~~~~~~~~~~
../../../libgcrypt/mpi/ec-nist.c:192:3: warning: ISO C90 forbids variable length array 'd' [-Wvla]
192 | mpi_limb64_t d[wsize];
| ^~~~~~~~~~~~
../../../libgcrypt/mpi/ec-nist.c: In function '_gcry_mpi_ec_nist256_mod':
../../../libgcrypt/mpi/ec-nist.c:350:3: warning: ISO C90 forbids variable length array 's' [-Wvla]
350 | mpi_limb64_t s[wsize + 1];
| ^~~~~~~~~~~~
../../../libgcrypt/mpi/ec-nist.c:351:3: warning: ISO C90 forbids variable length array 't' [-Wvla]
351 | mpi_limb64_t t[wsize + 1];
| ^~~~~~~~~~~~
../../../libgcrypt/mpi/ec-nist.c:352:3: warning: ISO C90 forbids variable length array 'd' [-Wvla]
352 | mpi_limb64_t d[wsize + 1];
| ^~~~~~~~~~~~
../../../libgcrypt/mpi/ec-nist.c:353:3: warning: ISO C90 forbids variable length array 'e' [-Wvla]
353 | mpi_limb64_t e[wsize + 1];
| ^~~~~~~~~~~~
../../../libgcrypt/mpi/ec-nist.c: In function '_gcry_mpi_ec_nist384_mod':
../../../libgcrypt/mpi/ec-nist.c:603:3: warning: ISO C90 forbids variable length array 's' [-Wvla]
603 | mpi_limb64_t s[wsize + 1];
| ^~~~~~~~~~~~
../../../libgcrypt/mpi/ec-nist.c:604:3: warning: ISO C90 forbids variable length array 't' [-Wvla]
604 | mpi_limb64_t t[wsize + 1];
| ^~~~~~~~~~~~
../../../libgcrypt/mpi/ec-nist.c:605:3: warning: ISO C90 forbids variable length array 'd' [-Wvla]
605 | mpi_limb64_t d[wsize + 1];
| ^~~~~~~~~~~~
../../../libgcrypt/mpi/ec-nist.c:606:3: warning: ISO C90 forbids variable length array 'x' [-Wvla]
606 | mpi_limb64_t x[wsize + 1];
| ^~~~~~~~~~~~
../../../libgcrypt/mpi/ec-nist.c: In function '_gcry_mpi_ec_nist521_mod':
../../../libgcrypt/mpi/ec-nist.c:795:3: warning: ISO C90 forbids variable length array 's' [-Wvla]
795 | mpi_limb_t s[wsize];
| ^~~~~~~~~~
--

_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
https://lists.gnupg.org/mailman/listinfo/gcrypt-devel
Re: Uninteded Variable Length Array in ec-nist.c [ In reply to ]
jussi.kivilinna at iki
Sep 28, 2022, 11:23 AM
Post #2 of 5 (449 views)
Permalink
Hello,

On 27.9.2022 3.54, NIIBE Yutaka wrote:
> Hello,
>
> While we allow use of some features of C99 for libgcrypt, we don't use
> variable length array in the code.
>
> Thus, I'm considering adding -Wvla option in configure.ac. In master,
> I found use of variable length array in ec-nist.c. I attach the
> warning message of compiler when it is compiled with -Wvla option.
>
> I think that it's not intended. Unfortunately, even if we added const
> qualifier to the variable 'wsize', (because what is needed here is a
> constant expression), it is still considered as variable length array by
> compilers.
>
> I think that use of macro for the size is needed here, although it would
> not look modern code.

How about instead define arrays with wanted size and define 'wsize' with
sizeof the array. This would avoid having macros. For example like this:

index 69b05a6d..0de41e48 100644
--- a/mpi/ec-nist.c
+++ b/mpi/ec-nist.c
@@ -94,9 +94,9 @@ _gcry_mpi_ec_nist192_mod (gcry_mpi_t w, mpi_ec_t ctx)
};
const mpi_limb64_t zero = LIMB_TO64(0);
mpi_ptr_t wp;
- mpi_size_t wsize = 192 / BITS_PER_MPI_LIMB64;
- mpi_limb64_t s[wsize + 1];
- mpi_limb64_t o[wsize + 1];
+ mpi_limb64_t s[192 / BITS_PER_MPI_LIMB64 + 1];
+ mpi_limb64_t o[sizeof(s)];
+ const mpi_size_t wsize = DIM(s) - 1;
mpi_limb_t mask1;
mpi_limb_t mask2;
mpi_limb_t s_is_negative;


If we want to, we can get rid of VLA in __gcry_burn_stack too. For
example, following should work (avoids GCC from turning recursive
call to loop and wiping same 64-byte memory over and over again):

void NOINLINE_FUNC
__gcry_burn_stack_recursive (unsigned int bytes)
{
__gcry_burn_stack(bytes);
}


void NOINLINE_FUNC
__gcry_burn_stack (unsigned int bytes)
{
char buf[64];

_gcry_fast_wipememory (buf, sizeof buf);

if (bytes > sizeof buf)
__gcry_burn_stack_recursive (bytes - sizeof buf);
}

'__gcry_burn_stack_recursive' could be moved to separate source
file just in case compiler does not support noinline function
attribute.

-Jussi


_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
https://lists.gnupg.org/mailman/listinfo/gcrypt-devel
Re: Uninteded Variable Length Array in ec-nist.c [ In reply to ]
gniibe at fsij
Sep 30, 2022, 1:14 AM
Post #3 of 5 (445 views)
Permalink
Thank you for your quick response.

Jussi Kivilinna <jussi.kivilinna@iki.fi> wrote:
> How about instead define arrays with wanted size and define 'wsize' with
> sizeof the array. This would avoid having macros. For example like this:
>
> index 69b05a6d..0de41e48 100644
> --- a/mpi/ec-nist.c
> +++ b/mpi/ec-nist.c
> @@ -94,9 +94,9 @@ _gcry_mpi_ec_nist192_mod (gcry_mpi_t w, mpi_ec_t ctx)
> };
> const mpi_limb64_t zero = LIMB_TO64(0);
> mpi_ptr_t wp;
> - mpi_size_t wsize = 192 / BITS_PER_MPI_LIMB64;
> - mpi_limb64_t s[wsize + 1];
> - mpi_limb64_t o[wsize + 1];
> + mpi_limb64_t s[192 / BITS_PER_MPI_LIMB64 + 1];
> + mpi_limb64_t o[sizeof(s)];
> + const mpi_size_t wsize = DIM(s) - 1;
> mpi_limb_t mask1;
> mpi_limb_t mask2;
> mpi_limb_t s_is_negative;

Looks nice with no macro definition. I like it.

> If we want to, we can get rid of VLA in __gcry_burn_stack too.

I think that __gcry_burn_stack is OK with VLA, because it's quite
special use case and !HAVE_VLA version is available.
--

_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
https://lists.gnupg.org/mailman/listinfo/gcrypt-devel
Re: Uninteded Variable Length Array in ec-nist.c [ In reply to ]
gcrypt-devel at gnupg
Sep 30, 2022, 4:36 AM
Post #4 of 5 (442 views)
Permalink
On Fri, Sep 30, 2022 at 05:14:16PM +0900, NIIBE Yutaka wrote:
> Thank you for your quick response.
>
> Jussi Kivilinna <jussi.kivilinna@iki.fi> wrote:
> > How about instead define arrays with wanted size and define 'wsize' with
> > sizeof the array. This would avoid having macros. For example like this:
> >
> > index 69b05a6d..0de41e48 100644
> > --- a/mpi/ec-nist.c
> > +++ b/mpi/ec-nist.c
> > @@ -94,9 +94,9 @@ _gcry_mpi_ec_nist192_mod (gcry_mpi_t w, mpi_ec_t ctx)
> > };
> > const mpi_limb64_t zero = LIMB_TO64(0);
> > mpi_ptr_t wp;
> > - mpi_size_t wsize = 192 / BITS_PER_MPI_LIMB64;
> > - mpi_limb64_t s[wsize + 1];
> > - mpi_limb64_t o[wsize + 1];
> > + mpi_limb64_t s[192 / BITS_PER_MPI_LIMB64 + 1];
> > + mpi_limb64_t o[sizeof(s)];

Note that sizeof(s) is the number of *bytes* of s, not the number of
*elements* of s, so the above new code will declare o to be much larger
than the old code did.

_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
https://lists.gnupg.org/mailman/listinfo/gcrypt-devel
Re: Uninteded Variable Length Array in ec-nist.c [ In reply to ]
jussi.kivilinna at iki
Sep 30, 2022, 11:15 PM
Post #5 of 5 (432 views)
Permalink
On 30.9.2022 14.36, Ian Goldberg via Gcrypt-devel wrote:
> On Fri, Sep 30, 2022 at 05:14:16PM +0900, NIIBE Yutaka wrote:
>> Thank you for your quick response.
>>
>> Jussi Kivilinna <jussi.kivilinna@iki.fi> wrote:
>>> How about instead define arrays with wanted size and define 'wsize' with
>>> sizeof the array. This would avoid having macros. For example like this:
>>>
>>> index 69b05a6d..0de41e48 100644
>>> --- a/mpi/ec-nist.c
>>> +++ b/mpi/ec-nist.c
>>> @@ -94,9 +94,9 @@ _gcry_mpi_ec_nist192_mod (gcry_mpi_t w, mpi_ec_t ctx)
>>> };
>>> const mpi_limb64_t zero = LIMB_TO64(0);
>>> mpi_ptr_t wp;
>>> - mpi_size_t wsize = 192 / BITS_PER_MPI_LIMB64;
>>> - mpi_limb64_t s[wsize + 1];
>>> - mpi_limb64_t o[wsize + 1];
>>> + mpi_limb64_t s[192 / BITS_PER_MPI_LIMB64 + 1];
>>> + mpi_limb64_t o[sizeof(s)];
>
> Note that sizeof(s) is the number of *bytes* of s, not the number of
> *elements* of s, so the above new code will declare o to be much larger
> than the old code did.

Thanks, I somehow missed that. Next line in my example used DIM macro
which does the right thing of giving number of elements in array.

+ const mpi_size_t wsize = DIM(s) - 1;

Just need to change to use DIM for array definitions too.

-Jussi


_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
https://lists.gnupg.org/mailman/listinfo/gcrypt-devel

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal