| Message ID | 05fe3295-7993-2eb5-37bc-a6d04fde1fbc@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11 | expand |
Hi Jan, On 01/03/2021 07:57, Jan Beulich wrote: > The upcoming release complains, not entirely unreasonably: > > In file included from rijndael.c:33: > .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]' > 55 | void rijndaelEncrypt(const unsigned int [], int, const unsigned char [], > | ^~~~~~~~~~~~~~~~~~~~~~ > rijndael.c:865:8: error: argument 4 of type 'u8[16]' {aka 'unsigned char[16]'} with mismatched bound [-Werror=array-parameter=] > 865 | u8 ct[16]) > | ~~~^~~~~~ > In file included from rijndael.c:33: > .../xen/include/crypto/rijndael.h:56:13: note: previously declared as 'unsigned char[]' > 56 | unsigned char []); > | ^~~~~~~~~~~~~~~~ > > While it's not really clear to me why it would complain only for arg 4, > the adjustment to make is obvious and riskfree also for arg 3: Simply > declare the correct array dimension right away. This then allows > compilers to apply checking at call sites, which seems desirable anyway. I am a bit confused, if GCC is not complaining for arg3, then what is the following error message for: > In file included from rijndael.c:33: > .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]' > 55 | void rijndaelEncrypt(const unsigned int [], int, const unsigned char [], > | ^~~~~~~~~~~~~~~~~~~~~~ > For the moment I'm leaving untouched the disagreement between u8/u32 > used in the function definition and unsigned {char,int} used in the > declaration, as making this consistent would call for touching further > functions. > > Reported-by: Charles Arnold <carnold@suse.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > There are quite a few more issues with gcc11, but from my brief initial > inspection I'm suspecting (hoping) it'll rather be the compiler which > will get further changed by the time their release gets finalized. Just > one example: > > .../xen/include/xen/string.h:101:27: error: '__builtin_memcmp' specified bound 4 exceeds source size 0 [-Werror=stringop-overread] > 101 | #define memcmp(s1, s2, n) __builtin_memcmp(s1, s2, n) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > mpparse.c:722:13: note: in expansion of macro 'memcmp' > 722 | if (memcmp(mpf->mpf_signature, "_MP_", 4) == 0 && > | ^~~~~~ > > Clearly neither the 1st nor the 2nd argument have a "source size" of 0. It looks like there is a report on the redhat bug tracker for it [1]. Do you know if there is a bug report on the GCC tracker as well? > > --- a/xen/include/crypto/rijndael.h > +++ b/xen/include/crypto/rijndael.h > @@ -52,7 +52,7 @@ > > int rijndaelKeySetupEnc(unsigned int [], const unsigned char [], int); > int rijndaelKeySetupDec(unsigned int [], const unsigned char [], int); > -void rijndaelEncrypt(const unsigned int [], int, const unsigned char [], > - unsigned char []); > +void rijndaelEncrypt(const unsigned int [], int, const unsigned char [16], > + unsigned char [16]); > > #endif /* __RIJNDAEL_H */ > Cheers, [1] https://bugzilla.redhat.com/show_bug.cgi?id=1892100
On 03.03.2021 20:09, Julien Grall wrote: > On 01/03/2021 07:57, Jan Beulich wrote: >> The upcoming release complains, not entirely unreasonably: >> >> In file included from rijndael.c:33: >> .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]' >> 55 | void rijndaelEncrypt(const unsigned int [], int, const unsigned char [], >> | ^~~~~~~~~~~~~~~~~~~~~~ >> rijndael.c:865:8: error: argument 4 of type 'u8[16]' {aka 'unsigned char[16]'} with mismatched bound [-Werror=array-parameter=] >> 865 | u8 ct[16]) >> | ~~~^~~~~~ >> In file included from rijndael.c:33: >> .../xen/include/crypto/rijndael.h:56:13: note: previously declared as 'unsigned char[]' >> 56 | unsigned char []); >> | ^~~~~~~~~~~~~~~~ >> >> While it's not really clear to me why it would complain only for arg 4, >> the adjustment to make is obvious and riskfree also for arg 3: Simply >> declare the correct array dimension right away. This then allows >> compilers to apply checking at call sites, which seems desirable anyway. > > I am a bit confused, if GCC is not complaining for arg3, then what is > the following error message for: > > > In file included from rijndael.c:33: > > .../xen/include/crypto/rijndael.h:55:53: note: previously declared as > 'const unsigned char[]' > > 55 | void rijndaelEncrypt(const unsigned int [], int, const > unsigned char [], > > | > ^~~~~~~~~~~~~~~~~~~~~~ Hmm, good point. I didn't observe this myself, and simply copied the part of the error message that I was handed. I suppose there was an "error: argument 3 of type ..." there then as well. Charles - any chance you could confirm this, and perhaps even quote the full set of error messages in our internal patch? I'll adjust the wording of the description in any event. >> There are quite a few more issues with gcc11, but from my brief initial >> inspection I'm suspecting (hoping) it'll rather be the compiler which >> will get further changed by the time their release gets finalized. Just >> one example: >> >> .../xen/include/xen/string.h:101:27: error: '__builtin_memcmp' specified bound 4 exceeds source size 0 [-Werror=stringop-overread] >> 101 | #define memcmp(s1, s2, n) __builtin_memcmp(s1, s2, n) >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ >> mpparse.c:722:13: note: in expansion of macro 'memcmp' >> 722 | if (memcmp(mpf->mpf_signature, "_MP_", 4) == 0 && >> | ^~~~~~ >> >> Clearly neither the 1st nor the 2nd argument have a "source size" of 0. > > It looks like there is a report on the redhat bug tracker for it [1]. Do > you know if there is a bug report on the GCC tracker as well? I have no idea, to be honest. Jan
Jan Beulich writes ("Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"):
> On 03.03.2021 20:09, Julien Grall wrote:
> > On 01/03/2021 07:57, Jan Beulich wrote:
> >> The upcoming release complains, not entirely unreasonably:
> >>
> >> In file included from rijndael.c:33:
> >> .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]'
> >> 55 | void rijndaelEncrypt(const unsigned int [], int, const unsigned char [],
> >> | ^~~~~~~~~~~~~~~~~~~~~~
> >> rijndael.c:865:8: error: argument 4 of type 'u8[16]' {aka 'unsigned char[16]'} with mismatched bound [-Werror=array-parameter=]
> >> 865 | u8 ct[16])
I think this is an erroneous compiler warning.
It has been idiomatic in some codebases for a long time to write
const unsigned char[]
as a formal parameter for an array (of whatever size).
> >> .../xen/include/xen/string.h:101:27: error: '__builtin_memcmp' specified bound 4 exceeds source size 0 [-Werror=stringop-overread]
> >> 101 | #define memcmp(s1, s2, n) __builtin_memcmp(s1, s2, n)
> >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> mpparse.c:722:13: note: in expansion of macro 'memcmp'
> >> 722 | if (memcmp(mpf->mpf_signature, "_MP_", 4) == 0 &&
> >> | ^~~~~~
> >>
> >> Clearly neither the 1st nor the 2nd argument have a "source size" of 0.
> >
> > It looks like there is a report on the redhat bug tracker for it [1]. Do
> > you know if there is a bug report on the GCC tracker as well?
>
> I have no idea, to be honest.
This erroneous message makes me think that there is simply a bug in
this version of GCC, where formal parameters declared as
const unsigned char[]
are treated as
const unsigned char[0]
rather than as
const unsigned char*
Ian.
On 04/03/2021 08:06, Jan Beulich wrote: > On 03.03.2021 20:09, Julien Grall wrote: >> On 01/03/2021 07:57, Jan Beulich wrote: >>> The upcoming release complains, not entirely unreasonably: >>> >>> In file included from rijndael.c:33: >>> .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]' >>> 55 | void rijndaelEncrypt(const unsigned int [], int, const unsigned char [], >>> | ^~~~~~~~~~~~~~~~~~~~~~ >>> rijndael.c:865:8: error: argument 4 of type 'u8[16]' {aka 'unsigned char[16]'} with mismatched bound [-Werror=array-parameter=] >>> 865 | u8 ct[16]) >>> | ~~~^~~~~~ >>> In file included from rijndael.c:33: >>> .../xen/include/crypto/rijndael.h:56:13: note: previously declared as 'unsigned char[]' >>> 56 | unsigned char []); >>> | ^~~~~~~~~~~~~~~~ >>> >>> While it's not really clear to me why it would complain only for arg 4, >>> the adjustment to make is obvious and riskfree also for arg 3: Simply >>> declare the correct array dimension right away. This then allows >>> compilers to apply checking at call sites, which seems desirable anyway. >> >> I am a bit confused, if GCC is not complaining for arg3, then what is >> the following error message for: >> >> > In file included from rijndael.c:33: >> > .../xen/include/crypto/rijndael.h:55:53: note: previously declared as >> 'const unsigned char[]' >> > 55 | void rijndaelEncrypt(const unsigned int [], int, const >> unsigned char [], >> > | >> ^~~~~~~~~~~~~~~~~~~~~~ > > Hmm, good point. I didn't observe this myself, and simply copied the > part of the error message that I was handed. I suppose there was an > "error: argument 3 of type ..." there then as well. Charles - any > chance you could confirm this, and perhaps even quote the full set > of error messages in our internal patch? > > I'll adjust the wording of the description in any event. With the description adjusted: Reviewed-by: Julien Grall <jgrall@amazon.com> > >>> There are quite a few more issues with gcc11, but from my brief initial >>> inspection I'm suspecting (hoping) it'll rather be the compiler which >>> will get further changed by the time their release gets finalized. Just >>> one example: >>> >>> .../xen/include/xen/string.h:101:27: error: '__builtin_memcmp' specified bound 4 exceeds source size 0 [-Werror=stringop-overread] >>> 101 | #define memcmp(s1, s2, n) __builtin_memcmp(s1, s2, n) >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> mpparse.c:722:13: note: in expansion of macro 'memcmp' >>> 722 | if (memcmp(mpf->mpf_signature, "_MP_", 4) == 0 && >>> | ^~~~~~ >>> >>> Clearly neither the 1st nor the 2nd argument have a "source size" of 0. >> >> It looks like there is a report on the redhat bug tracker for it [1]. Do >> you know if there is a bug report on the GCC tracker as well? > > I have no idea, to be honest. I had a look and couldn't find any. It might be worth to fill one in case they are not aware. Cheers,
On 04/03/2021 11:21, Ian Jackson wrote: > Jan Beulich writes ("Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"): >> On 03.03.2021 20:09, Julien Grall wrote: >>> On 01/03/2021 07:57, Jan Beulich wrote: >>>> The upcoming release complains, not entirely unreasonably: >>>> >>>> In file included from rijndael.c:33: >>>> .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]' >>>> 55 | void rijndaelEncrypt(const unsigned int [], int, const unsigned char [], >>>> | ^~~~~~~~~~~~~~~~~~~~~~ >>>> rijndael.c:865:8: error: argument 4 of type 'u8[16]' {aka 'unsigned char[16]'} with mismatched bound [-Werror=array-parameter=] >>>> 865 | u8 ct[16]) > > I think this is an erroneous compiler warning. > > It has been idiomatic in some codebases for a long time to write > const unsigned char[] > as a formal parameter for an array (of whatever size). AFAICT, this is not what GCC is trying to warn about. It is complaining that the prototype and the declaration doesn't use the same signature. Indeed, the former is using [] while the declaration is using [16]. So compiler is working as intended when -Warray-parameter is selected (see [1]). [1] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
Julien Grall writes ("Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"):
> On 04/03/2021 11:21, Ian Jackson wrote:
> > Jan Beulich writes ("Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"):
...
> > It has been idiomatic in some codebases for a long time to write
> > const unsigned char[]
> > as a formal parameter for an array (of whatever size).
>
> AFAICT, this is not what GCC is trying to warn about. It is complaining
> that the prototype and the declaration doesn't use the same signature.
Oh! I would have to check whether that is legal (I would guess
probably it is UB because the C authors hate us all) but I agree that
the warning is justified and the code should be changed.
Sorry for the misunderstanding.
Ian.
On 04.03.2021 12:21, Ian Jackson wrote: > Jan Beulich writes ("Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"): >> On 03.03.2021 20:09, Julien Grall wrote: >>> On 01/03/2021 07:57, Jan Beulich wrote: >>>> The upcoming release complains, not entirely unreasonably: >>>> >>>> In file included from rijndael.c:33: >>>> .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]' >>>> 55 | void rijndaelEncrypt(const unsigned int [], int, const unsigned char [], >>>> | ^~~~~~~~~~~~~~~~~~~~~~ >>>> rijndael.c:865:8: error: argument 4 of type 'u8[16]' {aka 'unsigned char[16]'} with mismatched bound [-Werror=array-parameter=] >>>> 865 | u8 ct[16]) > > I think this is an erroneous compiler warning. > > It has been idiomatic in some codebases for a long time to write > const unsigned char[] > as a formal parameter for an array (of whatever size). > >>>> .../xen/include/xen/string.h:101:27: error: '__builtin_memcmp' specified bound 4 exceeds source size 0 [-Werror=stringop-overread] >>>> 101 | #define memcmp(s1, s2, n) __builtin_memcmp(s1, s2, n) >>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>> mpparse.c:722:13: note: in expansion of macro 'memcmp' >>>> 722 | if (memcmp(mpf->mpf_signature, "_MP_", 4) == 0 && >>>> | ^~~~~~ >>>> >>>> Clearly neither the 1st nor the 2nd argument have a "source size" of 0. >>> >>> It looks like there is a report on the redhat bug tracker for it [1]. Do >>> you know if there is a bug report on the GCC tracker as well? >> >> I have no idea, to be honest. > > This erroneous message makes me think that there is simply a bug in > this version of GCC, where formal parameters declared as > const unsigned char[] > are treated as > const unsigned char[0] > rather than as > const unsigned char* I don't think so, no. In addition to what Julien has said, I think the intention here is that the compiler can check that both consumer and producers of arguments obey to the strictest available bounds of such an array, no matter that at the syntactic level char[N] and char[] both convert to char* when used in prototypes. Jan
On 04.03.2021 13:41, Ian Jackson wrote: > Julien Grall writes ("Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"): >> On 04/03/2021 11:21, Ian Jackson wrote: >>> Jan Beulich writes ("Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"): > ... >>> It has been idiomatic in some codebases for a long time to write >>> const unsigned char[] >>> as a formal parameter for an array (of whatever size). >> >> AFAICT, this is not what GCC is trying to warn about. It is complaining >> that the prototype and the declaration doesn't use the same signature. > > Oh! I would have to check whether that is legal (I would guess > probably it is UB because the C authors hate us all) but I agree that > the warning is justified and the code should be changed. May I translate this into a release ack then? Jan
Jan Beulich writes ("[PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"):
> Clearly neither the 1st nor the 2nd argument have a "source size" of 0.
>
> --- a/xen/include/crypto/rijndael.h
> +++ b/xen/include/crypto/rijndael.h
> @@ -52,7 +52,7 @@
>
> int rijndaelKeySetupEnc(unsigned int [], const unsigned char [], int);
> int rijndaelKeySetupDec(unsigned int [], const unsigned char [], int);
> -void rijndaelEncrypt(const unsigned int [], int, const unsigned char [],
> - unsigned char []);
> +void rijndaelEncrypt(const unsigned int [], int, const unsigned char [16],
> + unsigned char [16]);
>
> #endif /* __RIJNDAEL_H */
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
--- a/xen/include/crypto/rijndael.h +++ b/xen/include/crypto/rijndael.h @@ -52,7 +52,7 @@ int rijndaelKeySetupEnc(unsigned int [], const unsigned char [], int); int rijndaelKeySetupDec(unsigned int [], const unsigned char [], int); -void rijndaelEncrypt(const unsigned int [], int, const unsigned char [], - unsigned char []); +void rijndaelEncrypt(const unsigned int [], int, const unsigned char [16], + unsigned char [16]); #endif /* __RIJNDAEL_H */
The upcoming release complains, not entirely unreasonably: In file included from rijndael.c:33: .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]' 55 | void rijndaelEncrypt(const unsigned int [], int, const unsigned char [], | ^~~~~~~~~~~~~~~~~~~~~~ rijndael.c:865:8: error: argument 4 of type 'u8[16]' {aka 'unsigned char[16]'} with mismatched bound [-Werror=array-parameter=] 865 | u8 ct[16]) | ~~~^~~~~~ In file included from rijndael.c:33: .../xen/include/crypto/rijndael.h:56:13: note: previously declared as 'unsigned char[]' 56 | unsigned char []); | ^~~~~~~~~~~~~~~~ While it's not really clear to me why it would complain only for arg 4, the adjustment to make is obvious and riskfree also for arg 3: Simply declare the correct array dimension right away. This then allows compilers to apply checking at call sites, which seems desirable anyway. For the moment I'm leaving untouched the disagreement between u8/u32 used in the function definition and unsigned {char,int} used in the declaration, as making this consistent would call for touching further functions. Reported-by: Charles Arnold <carnold@suse.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- There are quite a few more issues with gcc11, but from my brief initial inspection I'm suspecting (hoping) it'll rather be the compiler which will get further changed by the time their release gets finalized. Just one example: .../xen/include/xen/string.h:101:27: error: '__builtin_memcmp' specified bound 4 exceeds source size 0 [-Werror=stringop-overread] 101 | #define memcmp(s1, s2, n) __builtin_memcmp(s1, s2, n) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ mpparse.c:722:13: note: in expansion of macro 'memcmp' 722 | if (memcmp(mpf->mpf_signature, "_MP_", 4) == 0 && | ^~~~~~ Clearly neither the 1st nor the 2nd argument have a "source size" of 0.