| Message ID | 20190220030912.14093-1-stewart@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libffs: Fix string truncation gcc warning. | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
On Wed, Feb 20, 2019 at 2:11 PM Stewart Smith <stewart@linux.ibm.com> wrote: > > From: Michal Suchanek <msuchanek@suse.de> > > Allow one more byte copied. The allocated space has extra byte anyway. > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > --- > libflash/libffs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libflash/libffs.c b/libflash/libffs.c > index 221c2b024c10..4eb0ffa997ac 100644 > --- a/libflash/libffs.c > +++ b/libflash/libffs.c > @@ -522,7 +522,7 @@ int ffs_part_info(struct ffs_handle *ffs, uint32_t part_idx, > n = calloc(1, FFS_PART_NAME_MAX + 1); > if (!n) > return FLASH_ERR_MALLOC_FAILED; > - strncpy(n, ent->name, FFS_PART_NAME_MAX); > + strncpy(n, ent->name, FFS_PART_NAME_MAX + 1); Eh... If the partition name uses all 16 bytes of the name (i.e it's not terminated) then ent->name will be left unterminated with this change. The comments on ffs_entry are correct the name field is supposed to be terminated anyway so only copying FFS_PART_NAME_MAX (15) bytes is better since it doesn't require the FFS input to be well formed. > *name = n; > } > return 0; > -- > 2.20.1 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
On Wed, Feb 20, 2019 at 3:10 PM Oliver <oohall@gmail.com> wrote: > > On Wed, Feb 20, 2019 at 2:11 PM Stewart Smith <stewart@linux.ibm.com> wrote: > > > > From: Michal Suchanek <msuchanek@suse.de> > > > > Allow one more byte copied. The allocated space has extra byte anyway. > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > > --- > > libflash/libffs.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libflash/libffs.c b/libflash/libffs.c > > index 221c2b024c10..4eb0ffa997ac 100644 > > --- a/libflash/libffs.c > > +++ b/libflash/libffs.c > > @@ -522,7 +522,7 @@ int ffs_part_info(struct ffs_handle *ffs, uint32_t part_idx, > > n = calloc(1, FFS_PART_NAME_MAX + 1); > > if (!n) > > return FLASH_ERR_MALLOC_FAILED; > > - strncpy(n, ent->name, FFS_PART_NAME_MAX); > > + strncpy(n, ent->name, FFS_PART_NAME_MAX + 1); > > Eh... If the partition name uses all 16 bytes of the name (i.e it's > not terminated) then ent->name will be left unterminated with this > change. The comments on ffs_entry are correct the name field is > supposed to be terminated anyway so only copying FFS_PART_NAME_MAX > (15) bytes is better since it doesn't require the FFS input to be well > formed. s/ent->name/n/ > > > *name = n; > > } > > return 0; > > -- > > 2.20.1 > > > > _______________________________________________ > > Skiboot mailing list > > Skiboot@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/skiboot
On 02/20/2019 09:40 AM, Oliver wrote: > On Wed, Feb 20, 2019 at 2:11 PM Stewart Smith <stewart@linux.ibm.com> wrote: >> >> From: Michal Suchanek <msuchanek@suse.de> >> >> Allow one more byte copied. The allocated space has extra byte anyway. >> >> Signed-off-by: Michal Suchanek <msuchanek@suse.de> >> --- >> libflash/libffs.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libflash/libffs.c b/libflash/libffs.c >> index 221c2b024c10..4eb0ffa997ac 100644 >> --- a/libflash/libffs.c >> +++ b/libflash/libffs.c >> @@ -522,7 +522,7 @@ int ffs_part_info(struct ffs_handle *ffs, uint32_t part_idx, >> n = calloc(1, FFS_PART_NAME_MAX + 1); >> if (!n) >> return FLASH_ERR_MALLOC_FAILED; >> - strncpy(n, ent->name, FFS_PART_NAME_MAX); >> + strncpy(n, ent->name, FFS_PART_NAME_MAX + 1); > > Eh... If the partition name uses all 16 bytes of the name (i.e it's > not terminated) then ent->name will be left unterminated with this > change. The comments on ffs_entry are correct the name field is > supposed to be terminated anyway so only copying FFS_PART_NAME_MAX > (15) bytes is better since it doesn't require the FFS input to be well > formed. Looks like we make sure ent->name is NULL terminate. Just to be safer we can explicitly NULL terminate `n` here. n[ FFS_PART_NAME_MAX] = '\0'; -Vasant
diff --git a/libflash/libffs.c b/libflash/libffs.c index 221c2b024c10..4eb0ffa997ac 100644 --- a/libflash/libffs.c +++ b/libflash/libffs.c @@ -522,7 +522,7 @@ int ffs_part_info(struct ffs_handle *ffs, uint32_t part_idx, n = calloc(1, FFS_PART_NAME_MAX + 1); if (!n) return FLASH_ERR_MALLOC_FAILED; - strncpy(n, ent->name, FFS_PART_NAME_MAX); + strncpy(n, ent->name, FFS_PART_NAME_MAX + 1); *name = n; } return 0;