We could always switch it to strncpy :) In any case - he is correct, it is better than what was there because we should not strcat unless the array were locked across the whole function On Mon, May 6, 2019 at 11:57 AM Jeremy Allison <jra@xxxxxxxxx> wrote: > > On Mon, May 06, 2019 at 11:53:44AM -0500, Steve French via samba-technical wrote: > > I think strcpy is clearer - but I don't think it can overflow since if > > R, W or W were written to "message" then cinode->oplock would be > > non-zero so we would never strcap "None" > > Ahem. In Samba we have : > > lib/util/safe_string.h:#define strcpy(dest,src) __ERROR__XX__NEVER_USE_STRCPY___; > > Maybe you should do likewise :-). > > > On Mon, May 6, 2019 at 10:26 AM Christoph Probst <kernel@xxxxxxxxx> wrote: > > > > > > Change strcat to strcpy in the "None" case as it is never valid to append > > > "None" to any other message. It may also overflow char message[5], in a > > > race condition on cinode if cinode->oplock is unset by another thread > > > after "RHW" or "RH" had been written to message. > > > > > > Signed-off-by: Christoph Probst <kernel@xxxxxxxxx> > > > --- > > > fs/cifs/smb2ops.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > > > index c36ff0d..5fd5567 100644 > > > --- a/fs/cifs/smb2ops.c > > > +++ b/fs/cifs/smb2ops.c > > > @@ -2936,7 +2936,7 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock, > > > strcat(message, "W"); > > > } > > > if (!cinode->oplock) > > > - strcat(message, "None"); > > > + strcpy(message, "None"); > > > cifs_dbg(FYI, "%s Lease granted on inode %p\n", message, > > > &cinode->vfs_inode); > > > } > > > -- > > > 2.1.4 > > > > > > > > > -- > > Thanks, > > > > Steve > > -- Thanks, Steve
