| Message ID | 1500044188.2662.4.camel@wdc.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
Hi Bart, On Fri, 2017-07-14 at 14:56 +0000, Bart Van Assche wrote: > On Fri, 2017-07-14 at 13:32 +0200, Martin Wilck wrote: > > If the first WWID_LEN bytes of the uuid_attribute do not contain > > a 0 byte, pp->wwid may end up not properly terminated. Fix it. > > > > Signed-off-by: Martin Wilck <mwilck@suse.com> > > --- > > libmultipath/discovery.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c > > index 663c8eaa..9951af84 100644 > > --- a/libmultipath/discovery.c > > +++ b/libmultipath/discovery.c > > @@ -1615,6 +1615,7 @@ get_udev_uid(struct path * pp, char > > *uid_attribute, struct udev_device *udev) > > len = strlen(value); > > } > > strncpy(pp->wwid, value, len); > > + pp->wwid[WWID_SIZE - 1] = '\0'; > > } else { > > condlog(3, "%s: no %s attribute", pp->dev, > > uid_attribute); > > Hi Martin, > > Your patch does not cause all overflows to be reported. I'm not sure what you mean. The overflow message is printed if and only if (strlen(value) + 1 > WWID_SIZE), which is correct, AFAICS. The point of my patch is just to avoid that multipath crashes later due to an unterminated string caused by this overflow. > How about using the > following (untested) alternative? > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c > index eca4ce97..80d962e6 100644 > --- a/libmultipath/discovery.c > +++ b/libmultipath/discovery.c > @@ -1607,13 +1607,8 @@ get_udev_uid(struct path * pp, char > *uid_attribute, struct udev_device *udev) > if (!value || strlen(value) == 0) > value = getenv(uid_attribute); > if (value && strlen(value)) { > - if (strlen(value) + 1 > WWID_SIZE) { > + if (strlcpy(pp->wwid, value, sizeof(pp->wwid)) >= > WWID_SIZE) > condlog(0, "%s: wwid overflow", pp->dev); > - len = WWID_SIZE; > - } else { > - len = strlen(value); > - } > - strncpy(pp->wwid, value, len); > } else { > condlog(3, "%s: no %s attribute", pp->dev, > uid_attribute); > Bart. Let's have a strncpy vs. strlcpy discussion :D ! I can do this if you insist, but I don't see a big benefit. We've tested with the patch I submitted. Thanks, Martin -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Fri, 2017-07-14 at 21:21 +0200, Martin Wilck wrote: > On Fri, 2017-07-14 at 14:56 +0000, Bart Van Assche wrote: > > How about using the following (untested) alternative? > > > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c > > index eca4ce97..80d962e6 100644 > > --- a/libmultipath/discovery.c > > +++ b/libmultipath/discovery.c > > @@ -1607,13 +1607,8 @@ get_udev_uid(struct path * pp, char > > *uid_attribute, struct udev_device *udev) > > if (!value || strlen(value) == 0) > > value = getenv(uid_attribute); > > if (value && strlen(value)) { > > - if (strlen(value) + 1 > WWID_SIZE) { > > + if (strlcpy(pp->wwid, value, sizeof(pp->wwid)) >= > > WWID_SIZE) > > condlog(0, "%s: wwid overflow", pp->dev); > > - len = WWID_SIZE; > > - } else { > > - len = strlen(value); > > - } > > - strncpy(pp->wwid, value, len); > > } else { > > condlog(3, "%s: no %s attribute", pp->dev, > > uid_attribute); > > Let's have a strncpy vs. strlcpy discussion :D ! > > I can do this if you insist, but I don't see a big benefit. We've > tested with the patch I submitted. My comments were not intended as an invitation to open a strncpy() vs. strlcpy() discussion. What I wanted to illustrate with the above patch is that when using strlcpy() it is not necessary to explicitly zero-terminate a string because strlcpy() guarantees zero-termination. Compact code that is as readable as more verbose code is always better because compact code is easier to verify. Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Fri, 2017-07-14 at 20:21 +0000, Bart Van Assche wrote: > On Fri, 2017-07-14 at 21:21 +0200, Martin Wilck wrote: > > > > Let's have a strncpy vs. strlcpy discussion :D ! > > > > I can do this if you insist, but I don't see a big benefit. We've > > tested with the patch I submitted. > > My comments were not intended as an invitation to open a strncpy() > vs. strlcpy() > discussion. What I wanted to illustrate with the above patch is that > when using > strlcpy() it is not necessary to explicitly zero-terminate a string > because > strlcpy() guarantees zero-termination. Compact code that is as > readable as more > verbose code is always better because compact code is easier to > verify. OK. I'll wait for comments on the other patches, and change v2 of this patch to your version. Martin -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Fri, 2017-07-14 at 23:21 +0200, Martin Wilck wrote: > OK. I'll wait for comments on the other patches, and change v2 of this > patch to your version. Hello Martin, Patch 1/4 of this series is the only patch I wanted to comment on. The other patches touch code that I'm not familiar enough with to review them. Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index eca4ce97..80d962e6 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -1607,13 +1607,8 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev) if (!value || strlen(value) == 0) value = getenv(uid_attribute); if (value && strlen(value)) { - if (strlen(value) + 1 > WWID_SIZE) { + if (strlcpy(pp->wwid, value, sizeof(pp->wwid)) >= WWID_SIZE) condlog(0, "%s: wwid overflow", pp->dev); - len = WWID_SIZE; - } else { - len = strlen(value); - } - strncpy(pp->wwid, value, len); } else { condlog(3, "%s: no %s attribute", pp->dev, uid_attribute);