Hi, > -----Original Message----- > From: [email protected] [mailto:linux-usb- > [email protected]] On Behalf Of [email protected] > Sent: Friday, May 09, 2014 2:51 PM > To: [email protected]; [email protected]; > [email protected] > Cc: Philippe De Swert > Subject: [PATCH 1/3] libusbg: Fix readlink/buffer overrun issue. > CID#56130, CID#56129 > > From: Philippe De Swert <[email protected]> > > Readlink can return the total length of the buffer (here 4096), so > we do not > want to dereference target[4096] as that would give an off by one > error. > > Signed-off-by: Philippe De Swert <[email protected]> > --- > src/usbg.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/src/usbg.c b/src/usbg.c > index d73943c..c226731 100644 > --- a/src/usbg.c > +++ b/src/usbg.c > @@ -856,9 +856,10 @@ static int > usbg_parse_config_binding(usbg_config *c, char *bpath, > goto out; > } > > - /* readlink() don't add this, > - * so we have to do it manually */ > - target[nmb] = '\0'; > + /* readlink() doesn't add this, so we have to do it manually, > + * we need to take care to not overrun as readlink can return > + * the maximum buffer and have a off-by-one error */ > + target[nmb-1] = '\0';
I agree that there was a possible off-by-one-error and thank you for catching this I have missed it, but maybe it is not the best solution? When you put '\0' sign to target[nmb-1] you are solving off by one problem but you always lose the last sign in path so the whole path may be invalid or point to other file. Maybe it would be a better solution to alloc USBG_MAX_PATH_LENGTH + 1 array and keep target[nmb] = '\0'? -- BR's Krzysztof Opasiak -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
