| Message ID | 1363346785-25119-1-git-send-email-colin.king@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, 2013-03-15 at 11:26 +0000, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Curiously, we are getting the following error on older versions > of GCC: > > cc1: warnings being treated as errors > uefi/uefirtvariable/uefirtvariable.c: In function 'getnextvariable_test2': > uefi/uefirtvariable/uefirtvariable.c:352:5: error: array subscript is above array bounds > > ..and it only happens on 32 bit builds. > > Looking at the compiled object code, it seems that: > > c = variablename[(len / sizeof(c)) - 1]; > > produces an add of 0xffffffff todo the subtraction of -1 > which triggers the array subscript array bounds warning, where as: > > uint64_t i = (len / sizeof(c)) - 1; > > c = variablename[i]; > > produces a subtraction of 1 and everthing is fine. > > This patch is a workaround. I've hand checked the result, and it > produces the code we require, so I think the original code was OK > but GCC was being overly zealous. > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > src/uefi/uefirtvariable/uefirtvariable.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c > index 7242afd..c5caa33 100644 > --- a/src/uefi/uefirtvariable/uefirtvariable.c > +++ b/src/uefi/uefirtvariable/uefirtvariable.c > @@ -349,7 +349,9 @@ static bool strlen_valid(uint16_t *variablename, uint64_t variablenamesize) > uint16_t c; > > for (len = 2; len <= variablenamesize; len += sizeof(c)) { > - c = variablename[(len / sizeof(c)) - 1]; > + uint64_t i = (len / sizeof(c)) - 1; > + > + c = variablename[i]; > if (!c) > break; > } Looks good to me. Thanks for fixing this up.
On 03/15/2013 07:26 PM, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Curiously, we are getting the following error on older versions > of GCC: > > cc1: warnings being treated as errors > uefi/uefirtvariable/uefirtvariable.c: In function 'getnextvariable_test2': > uefi/uefirtvariable/uefirtvariable.c:352:5: error: array subscript is above array bounds > > ..and it only happens on 32 bit builds. > > Looking at the compiled object code, it seems that: > > c = variablename[(len / sizeof(c)) - 1]; > > produces an add of 0xffffffff todo the subtraction of -1 > which triggers the array subscript array bounds warning, where as: > > uint64_t i = (len / sizeof(c)) - 1; > > c = variablename[i]; > > produces a subtraction of 1 and everthing is fine. > > This patch is a workaround. I've hand checked the result, and it > produces the code we require, so I think the original code was OK > but GCC was being overly zealous. > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > src/uefi/uefirtvariable/uefirtvariable.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c > index 7242afd..c5caa33 100644 > --- a/src/uefi/uefirtvariable/uefirtvariable.c > +++ b/src/uefi/uefirtvariable/uefirtvariable.c > @@ -349,7 +349,9 @@ static bool strlen_valid(uint16_t *variablename, uint64_t variablenamesize) > uint16_t c; > > for (len = 2; len <= variablenamesize; len += sizeof(c)) { > - c = variablename[(len / sizeof(c)) - 1]; > + uint64_t i = (len / sizeof(c)) - 1; > + > + c = variablename[i]; > if (!c) > break; > } > Acked-by: Ivan Hu <ivan.hu@canonical.com>
On 03/15/2013 07:26 PM, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Curiously, we are getting the following error on older versions > of GCC: > > cc1: warnings being treated as errors > uefi/uefirtvariable/uefirtvariable.c: In function 'getnextvariable_test2': > uefi/uefirtvariable/uefirtvariable.c:352:5: error: array subscript is above array bounds > > ..and it only happens on 32 bit builds. > > Looking at the compiled object code, it seems that: > > c = variablename[(len / sizeof(c)) - 1]; > > produces an add of 0xffffffff todo the subtraction of -1 > which triggers the array subscript array bounds warning, where as: > > uint64_t i = (len / sizeof(c)) - 1; > > c = variablename[i]; > > produces a subtraction of 1 and everthing is fine. > > This patch is a workaround. I've hand checked the result, and it > produces the code we require, so I think the original code was OK > but GCC was being overly zealous. > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > src/uefi/uefirtvariable/uefirtvariable.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c > index 7242afd..c5caa33 100644 > --- a/src/uefi/uefirtvariable/uefirtvariable.c > +++ b/src/uefi/uefirtvariable/uefirtvariable.c > @@ -349,7 +349,9 @@ static bool strlen_valid(uint16_t *variablename, uint64_t variablenamesize) > uint16_t c; > > for (len = 2; len <= variablenamesize; len += sizeof(c)) { > - c = variablename[(len / sizeof(c)) - 1]; > + uint64_t i = (len / sizeof(c)) - 1; > + > + c = variablename[i]; > if (!c) > break; > } > Acked-by: Alex Hung <alex.hung@canonical.com>
diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c index 7242afd..c5caa33 100644 --- a/src/uefi/uefirtvariable/uefirtvariable.c +++ b/src/uefi/uefirtvariable/uefirtvariable.c @@ -349,7 +349,9 @@ static bool strlen_valid(uint16_t *variablename, uint64_t variablenamesize) uint16_t c; for (len = 2; len <= variablenamesize; len += sizeof(c)) { - c = variablename[(len / sizeof(c)) - 1]; + uint64_t i = (len / sizeof(c)) - 1; + + c = variablename[i]; if (!c) break; }