Hi Tom, On 28 October 2016 at 14:04, Tom Rini <[email protected]> wrote: > On Fri, Oct 28, 2016 at 12:41:05PM -0700, Simon Glass wrote: >> Hi Tom, >> >> On 28 October 2016 at 11:59, Tom Rini <[email protected]> wrote: >> > On Thu, Oct 27, 2016 at 08:18:39PM -0600, Simon Glass wrote: >> >> Coverity complains that this can overflow. If we later increase the size >> >> of one of the strings in the table, it could happen. >> >> >> >> Adjust the code to protect against this. >> >> >> >> Signed-off-by: Simon Glass <[email protected]> >> >> Reported-by: Coverity (CID: 150964) >> >> --- >> >> >> >> Changes in v3: >> >> - Adjust to deal with what strncpy() actually does (I think) >> >> >> >> Changes in v2: >> >> - Drop unwanted #include >> >> >> >> common/image.c | 6 ++++-- >> >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/common/image.c b/common/image.c >> >> index 0e86c13..016f263 100644 >> >> --- a/common/image.c >> >> +++ b/common/image.c >> >> @@ -588,9 +588,11 @@ const table_entry_t *get_table_entry(const >> >> table_entry_t *table, int id) >> >> static const char *unknown_msg(enum ih_category category) >> >> { >> >> static char msg[30]; >> >> + static char unknown_str = "Unknown "; >> >> >> >> - strcpy(msg, "Unknown "); >> >> - strcat(msg, table_info[category].desc); >> >> + strcpy(msg, unknown_str); >> >> + strncat(msg, table_info[category].desc, >> >> + sizeof(msg) - sizeof(unknown_str)); >> > >> > We still need to subtract 1 more here at the end, for the NUL don't we? >> >> I was hoping that the sizeof(msg) would take care of that? > > No, and you didn't compile test this did you? ;) I was trying to throw > up a stupid test to confirm what all everything would be. > test.c:6:28: warning: initialization makes integer from pointer without a > cast [-Wint-conversion] > static char unknown_str = "Unknown "; > ^~~~~~~~~~ > test.c:6:28: error: initializer element is not computable at load time > > Correcting that to be *unknown_str gives us back the sizeof(char *) not > the string in question. So we need to use strlen(unknown_str), and > strlen does not include the NUL, so we would need to still add in the - > 1 after all of the above.
Sorry I was travelling last week and not really focussed on this. I meant to use []. > > And I'm being verbose above because string handling can be annoying and > I didn't get it 100% right in my head so I figured it's worth showing > the work. And since we're going with "Coverity says we've got string > problems" we should really correct it, and not just be wrong in a > different way :) I think I can use sizeof() and this time I've tested that it does the right thing, by adding a test function to call it. Regards, Simon _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
