| Message ID | 20190318014152.3635-1-stewart@linux.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hw/ipmi/test/run-fru: Fix string truncation warning, enhance test | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (2ba5ce84a197ee61423355f443a3ff3eea185ff1) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
On Mon, 2019-03-18 at 12:41 +1100, Stewart Smith wrote: > We've been getting this warning/error from recent GCC: > > In file included from hw/ipmi/test/run-fru.c:22: > hw/ipmi/test/../ipmi-fru.c: In function ‘fru_add’: > hw/ipmi/test/../ipmi-fru.c:162:3: warning: ‘strncpy’ output truncated copying > 32 bytes from a string of length 38 [-Wstringop-truncation] > strncpy(info.version, version, MAX_STR_LEN + 1); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > This patch does two things: > 1) Re-arrange some code to shut GCC up. > 2) Add extra fu to tests to ensure we're producing correct bytes. > > Signed-off-by: Stewart Smith <stewart@linux.ibm.com> Tested-by: Michael Neuling <mikey@neuling.org> > --- > hw/ipmi/ipmi-fru.c | 16 +++++++++------- > hw/ipmi/test/run-fru.c | 11 ++++++++++- > 2 files changed, 19 insertions(+), 8 deletions(-) > > diff --git a/hw/ipmi/ipmi-fru.c b/hw/ipmi/ipmi-fru.c > index ddcb3d6f8bc1..7e7f0a68c301 100644 > --- a/hw/ipmi/ipmi-fru.c > +++ b/hw/ipmi/ipmi-fru.c > @@ -129,8 +129,8 @@ static int fru_fill_product_info(u8 *buf, struct > product_info *info, size_t size > static int fru_add(u8 *buf, int size) > { > int len; > - char short_version[MAX_STR_LEN + 1]; > struct common_header common_hdr; > + char *short_version; > struct product_info info = { > .manufacturer = (char *) "IBM", > .product = (char *) "skiboot", > @@ -155,17 +155,19 @@ static int fru_add(u8 *buf, int size) > common_hdr.checksum = fru_checksum((u8 *) &common_hdr, > sizeof(common_hdr) - 1); > memcpy(buf, &common_hdr, sizeof(common_hdr)); > > + short_version = strdup(version); > info.version = short_version; > if (!strncmp(version, "skiboot-", 8)) > - strncpy(info.version, &version[8], MAX_STR_LEN + 1); > - else > - strncpy(info.version, version, MAX_STR_LEN + 1); > + info.version = &short_version[8]; > > - if (info.version[MAX_STR_LEN] != '\0') > - info.version[MAX_STR_LEN - 1] = '+'; > - info.version[MAX_STR_LEN] = '\0'; > + if (strlen(info.version) >= MAX_STR_LEN) { > + if (info.version[MAX_STR_LEN] != '\0') > + info.version[MAX_STR_LEN - 1] = '+'; > + info.version[MAX_STR_LEN] = '\0'; > + } > > len = fru_fill_product_info(&buf[64], &info, size - 64); > + free(short_version); > if (len < 0) > return OPAL_PARAMETER; > > diff --git a/hw/ipmi/test/run-fru.c b/hw/ipmi/test/run-fru.c > index 9f65ed459a3b..ff8df3912648 100644 > --- a/hw/ipmi/test/run-fru.c > +++ b/hw/ipmi/test/run-fru.c > @@ -21,6 +21,8 @@ > > #include "../ipmi-fru.c" > > +#include <string.h> > + > int error = 0; > > const char version[] = "a-too-long-version-test-string-is-here"; > @@ -88,7 +90,10 @@ int main(void) > buf = malloc(256); > > len = fru_fill_product_info(buf, &info, 40); > - assert(len > 0); > + assert(len == 40); > + assert(memcmp(buf, "\001\005\000\303IBM\307skiboot\305hello" > + "\30512345\30512345\304abcd\301-",len) == 0); > + > > /* Make sure the checksum is right */ > assert(!fru_checksum(buf, len)); > @@ -106,6 +111,10 @@ int main(void) > > memset(buf, 0, 256); > assert(fru_add(buf, 256) > 0); > + assert(0 == memcmp(&buf[64], "\001\a\000\303IBM\307skiboot\300" > + "\337a-too-long-version-test-string+\300\300\301" > + "\0\0\0",54)); > + > > memset(buf, 0, 256); > assert(fru_add(buf, 1) == OPAL_PARAMETER);
Stewart Smith <stewart@linux.ibm.com> writes: > We've been getting this warning/error from recent GCC: > > In file included from hw/ipmi/test/run-fru.c:22: > hw/ipmi/test/../ipmi-fru.c: In function ‘fru_add’: > hw/ipmi/test/../ipmi-fru.c:162:3: warning: ‘strncpy’ output truncated copying 32 bytes from a string of length 38 [-Wstringop-truncation] > strncpy(info.version, version, MAX_STR_LEN + 1); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > This patch does two things: > 1) Re-arrange some code to shut GCC up. > 2) Add extra fu to tests to ensure we're producing correct bytes. > > Signed-off-by: Stewart Smith <stewart@linux.ibm.com> > --- > hw/ipmi/ipmi-fru.c | 16 +++++++++------- > hw/ipmi/test/run-fru.c | 11 ++++++++++- > 2 files changed, 19 insertions(+), 8 deletions(-) Merged to master as of 043e85bf74774c9652c53c4d36dd7c7a3dffcd29
diff --git a/hw/ipmi/ipmi-fru.c b/hw/ipmi/ipmi-fru.c index ddcb3d6f8bc1..7e7f0a68c301 100644 --- a/hw/ipmi/ipmi-fru.c +++ b/hw/ipmi/ipmi-fru.c @@ -129,8 +129,8 @@ static int fru_fill_product_info(u8 *buf, struct product_info *info, size_t size static int fru_add(u8 *buf, int size) { int len; - char short_version[MAX_STR_LEN + 1]; struct common_header common_hdr; + char *short_version; struct product_info info = { .manufacturer = (char *) "IBM", .product = (char *) "skiboot", @@ -155,17 +155,19 @@ static int fru_add(u8 *buf, int size) common_hdr.checksum = fru_checksum((u8 *) &common_hdr, sizeof(common_hdr) - 1); memcpy(buf, &common_hdr, sizeof(common_hdr)); + short_version = strdup(version); info.version = short_version; if (!strncmp(version, "skiboot-", 8)) - strncpy(info.version, &version[8], MAX_STR_LEN + 1); - else - strncpy(info.version, version, MAX_STR_LEN + 1); + info.version = &short_version[8]; - if (info.version[MAX_STR_LEN] != '\0') - info.version[MAX_STR_LEN - 1] = '+'; - info.version[MAX_STR_LEN] = '\0'; + if (strlen(info.version) >= MAX_STR_LEN) { + if (info.version[MAX_STR_LEN] != '\0') + info.version[MAX_STR_LEN - 1] = '+'; + info.version[MAX_STR_LEN] = '\0'; + } len = fru_fill_product_info(&buf[64], &info, size - 64); + free(short_version); if (len < 0) return OPAL_PARAMETER; diff --git a/hw/ipmi/test/run-fru.c b/hw/ipmi/test/run-fru.c index 9f65ed459a3b..ff8df3912648 100644 --- a/hw/ipmi/test/run-fru.c +++ b/hw/ipmi/test/run-fru.c @@ -21,6 +21,8 @@ #include "../ipmi-fru.c" +#include <string.h> + int error = 0; const char version[] = "a-too-long-version-test-string-is-here"; @@ -88,7 +90,10 @@ int main(void) buf = malloc(256); len = fru_fill_product_info(buf, &info, 40); - assert(len > 0); + assert(len == 40); + assert(memcmp(buf, "\001\005\000\303IBM\307skiboot\305hello" + "\30512345\30512345\304abcd\301-",len) == 0); + /* Make sure the checksum is right */ assert(!fru_checksum(buf, len)); @@ -106,6 +111,10 @@ int main(void) memset(buf, 0, 256); assert(fru_add(buf, 256) > 0); + assert(0 == memcmp(&buf[64], "\001\a\000\303IBM\307skiboot\300" + "\337a-too-long-version-test-string+\300\300\301" + "\0\0\0",54)); + memset(buf, 0, 256); assert(fru_add(buf, 1) == OPAL_PARAMETER);
We've been getting this warning/error from recent GCC: In file included from hw/ipmi/test/run-fru.c:22: hw/ipmi/test/../ipmi-fru.c: In function ‘fru_add’: hw/ipmi/test/../ipmi-fru.c:162:3: warning: ‘strncpy’ output truncated copying 32 bytes from a string of length 38 [-Wstringop-truncation] strncpy(info.version, version, MAX_STR_LEN + 1); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This patch does two things: 1) Re-arrange some code to shut GCC up. 2) Add extra fu to tests to ensure we're producing correct bytes. Signed-off-by: Stewart Smith <stewart@linux.ibm.com> --- hw/ipmi/ipmi-fru.c | 16 +++++++++------- hw/ipmi/test/run-fru.c | 11 ++++++++++- 2 files changed, 19 insertions(+), 8 deletions(-)