| Message ID | 1643175325-31046-1-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | libmultipath: use asprintf() to allocate prefixed_uuid | expand |
On Tue, 2022-01-25 at 23:35 -0600, Benjamin Marzinski wrote: > gcc 12.0.1 failed building libmultipath due to a format-overflow > false > positive on 32-bit architectures. This isn't so surprising as > format-overflow=2 is very aggressive in the assumptions it makes > about > the arguments. Here, it assumes that mpp->wwid could take up all the > space that a pointer could point to, even if I add code to this > function > to explicitly null terminate mpp->wwid to fit in WWID_SIZE. This sounds like a bug in gcc which should be reported. > > To avoid this and simplify the function, switch from using calloc() > and > sprintf() to just using asprintf(). > > For reference, the gcc build error that this fixes is: > > devmapper.c: In function 'dm_addmap.constprop.0': > devmapper.h:27:21: error: '%s' directive writing up to 2147483644 > bytes into a region of size 2147483641 [-Werror=format-overflow=] > 27 | #define UUID_PREFIX "mpath-" > | ^~~~~~~~ > devmapper.c:484:53: note: format string is defined here > 484 | sprintf(prefixed_uuid, UUID_PREFIX "%s", mpp- > >wwid); > | ^~ > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> Reviewed-by: Martin Wilck <mwilck@suse.com> > --- > libmultipath/devmapper.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c > index 36038150..2507f77f 100644 > --- a/libmultipath/devmapper.c > +++ b/libmultipath/devmapper.c > @@ -473,14 +473,11 @@ dm_addmap (int task, const char *target, struct > multipath *mpp, > dm_task_set_ro(dmt); > > if (task == DM_DEVICE_CREATE) { > - prefixed_uuid = calloc(1, UUID_PREFIX_LEN + > - strlen(mpp->wwid) + 1); > - if (!prefixed_uuid) { > + if (asprintf(&prefixed_uuid, UUID_PREFIX "%s", mpp- > >wwid) < 0) { > condlog(0, "cannot create prefixed uuid : > %s", > strerror(errno)); > goto addout; > } > - sprintf(prefixed_uuid, UUID_PREFIX "%s", mpp->wwid); > if (!dm_task_set_uuid(dmt, prefixed_uuid)) > goto freeout; > dm_task_skip_lockfs(dmt); -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Wed, 2022-01-26 at 08:46 +0100, Martin Wilck wrote: > On Tue, 2022-01-25 at 23:35 -0600, Benjamin Marzinski wrote: > > gcc 12.0.1 failed building libmultipath due to a format-overflow > > false > > positive on 32-bit architectures. This isn't so surprising as > > format-overflow=2 is very aggressive in the assumptions it makes > > about > > the arguments. Here, it assumes that mpp->wwid could take up all > > the > > space that a pointer could point to, even if I add code to this > > function > > to explicitly null terminate mpp->wwid to fit in WWID_SIZE. > > This sounds like a bug in gcc which should be reported. ... and that bug exists already: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104119 Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c index 36038150..2507f77f 100644 --- a/libmultipath/devmapper.c +++ b/libmultipath/devmapper.c @@ -473,14 +473,11 @@ dm_addmap (int task, const char *target, struct multipath *mpp, dm_task_set_ro(dmt); if (task == DM_DEVICE_CREATE) { - prefixed_uuid = calloc(1, UUID_PREFIX_LEN + - strlen(mpp->wwid) + 1); - if (!prefixed_uuid) { + if (asprintf(&prefixed_uuid, UUID_PREFIX "%s", mpp->wwid) < 0) { condlog(0, "cannot create prefixed uuid : %s", strerror(errno)); goto addout; } - sprintf(prefixed_uuid, UUID_PREFIX "%s", mpp->wwid); if (!dm_task_set_uuid(dmt, prefixed_uuid)) goto freeout; dm_task_skip_lockfs(dmt);
gcc 12.0.1 failed building libmultipath due to a format-overflow false positive on 32-bit architectures. This isn't so surprising as format-overflow=2 is very aggressive in the assumptions it makes about the arguments. Here, it assumes that mpp->wwid could take up all the space that a pointer could point to, even if I add code to this function to explicitly null terminate mpp->wwid to fit in WWID_SIZE. To avoid this and simplify the function, switch from using calloc() and sprintf() to just using asprintf(). For reference, the gcc build error that this fixes is: devmapper.c: In function 'dm_addmap.constprop.0': devmapper.h:27:21: error: '%s' directive writing up to 2147483644 bytes into a region of size 2147483641 [-Werror=format-overflow=] 27 | #define UUID_PREFIX "mpath-" | ^~~~~~~~ devmapper.c:484:53: note: format string is defined here 484 | sprintf(prefixed_uuid, UUID_PREFIX "%s", mpp->wwid); | ^~ Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/devmapper.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)