| Message ID | 1345481724-30108-6-git-send-email-jim@meyering.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Bastien ROUCARIES wrote: > Le 20 août 2012 19:29, "Jim Meyering" <jim@meyering.net> a écrit : >> >> From: Jim Meyering <meyering@redhat.com> >> >> Each of the protocols[i].name strings (statically declared above) >> has length less than 16, so this use of strncpy is misleading: >> strncpy(pSMB->DialectsArray+count, protocols[i].name, 16); >> Besides, if a new name were added with length N >= 16, the existing >> strncpy-using code would be buggy, creating a ->DialectsArray buffer >> containing N-16+1 unset bytes where the NUL terminator should have >> been. Instead, traverse the name only once go get its length, >> use a BUG_ON assertion to enforce the length restriction >> and use memcpy to perform the copy. > > Could you use ARRAY_SIZE instead of hard coding 16? Not that I can see: the DialectsArray member is declared like this: typedef struct negotiate_req { struct smb_hdr hdr; /* wct = 0 */ __le16 ByteCount; unsigned char DialectsArray[1]; } __attribute__((packed)) NEGOTIATE_REQ; ... >> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c >> index 074923c..16a9018 100644 >> --- a/fs/cifs/cifssmb.c >> +++ b/fs/cifs/cifssmb.c >> @@ -441,8 +441,10 @@ CIFSSMBNegotiate(const unsigned int xid, struct cifs_ses > *ses) >> >> count = 0; >> for (i = 0; i < CIFS_NUM_PROT; i++) { >> - strncpy(pSMB->DialectsArray+count, protocols[i].name, 16); >> - count += strlen(protocols[i].name) + 1; >> + size_t len = strlen(protocols[i].name); >> + BUG_ON(len >= 16); >> + memcpy(pSMB->DialectsArray+count, protocols[i].name, len + 1); >> + count += len + 1; -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jim Meyering wrote: > From: Jim Meyering <meyering@redhat.com> > > Each of the protocols[i].name strings (statically declared above) > has length less than 16, so this use of strncpy is misleading: > strncpy(pSMB->DialectsArray+count, protocols[i].name, 16); > Besides, if a new name were added with length N >= 16, the existing > strncpy-using code would be buggy, creating a ->DialectsArray buffer > containing N-16+1 unset bytes where the NUL terminator should have > been. Instead, traverse the name only once go get its length, s/go/to/, of course. > use a BUG_ON assertion to enforce the length restriction > and use memcpy to perform the copy. > > Signed-off-by: Jim Meyering <meyering@redhat.com> > --- > fs/cifs/cifssmb.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > index 074923c..16a9018 100644 > --- a/fs/cifs/cifssmb.c > +++ b/fs/cifs/cifssmb.c > @@ -441,8 +441,10 @@ CIFSSMBNegotiate(const unsigned int xid, struct cifs_ses *ses) > > count = 0; > for (i = 0; i < CIFS_NUM_PROT; i++) { > - strncpy(pSMB->DialectsArray+count, protocols[i].name, 16); > - count += strlen(protocols[i].name) + 1; > + size_t len = strlen(protocols[i].name); > + BUG_ON(len >= 16); > + memcpy(pSMB->DialectsArray+count, protocols[i].name, len + 1); > + count += len + 1; > /* null at end of source and target buffers anyway */ > } > inc_rfc1001_len(pSMB, count); -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index 074923c..16a9018 100644 --- a/fs/cifs/cifssmb.c +++ b/fs/cifs/cifssmb.c @@ -441,8 +441,10 @@ CIFSSMBNegotiate(const unsigned int xid, struct cifs_ses *ses) count = 0; for (i = 0; i < CIFS_NUM_PROT; i++) { - strncpy(pSMB->DialectsArray+count, protocols[i].name, 16); - count += strlen(protocols[i].name) + 1; + size_t len = strlen(protocols[i].name); + BUG_ON(len >= 16); + memcpy(pSMB->DialectsArray+count, protocols[i].name, len + 1); + count += len + 1; /* null at end of source and target buffers anyway */ } inc_rfc1001_len(pSMB, count);