From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lidza Louina Date: Mon, 16 May 2016 10:11:52 -0400 Subject: [lustre-devel] [PATCH] staging/lustre/ptlrpc: Removes potential null dereference In-Reply-To: <7641B85C-A5E1-499C-9426-8A860900C35C@intel.com> References: <1463092392.16344.4.camel@oracle.com> <7641B85C-A5E1-499C-9426-8A860900C35C@intel.com> Message-ID: <5739D528.4000606@oracle.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: lustre-devel@lists.lustre.org On 05/13/2016 12:43 PM, Drokin, Oleg wrote: > On May 12, 2016, at 6:33 PM, Lidza Louina wrote: > >> The lustre_msg_buf method could return NULL. Subsequent code didn't >> check if it's null before using it. This patch adds two checks. > Thank you for the patch, it looks good code-wise, but there are some style issues. > >> >> Signed-off-by: Lidza Louina >> >> diff --git a/drivers/staging/lustre/lustre/ptlrpc/sec.c b/drivers/staging/lustre/lustre/ptlrpc/sec.c >> index 187fd1d..e6fedc3 100644 >> --- a/drivers/staging/lustre/lustre/ptlrpc/sec.c >> +++ b/drivers/staging/lustre/lustre/ptlrpc/sec.c >> @@ -2195,6 +2195,8 @@ int sptlrpc_pack_user_desc(struct lustre_msg *msg, int offset) >> struct ptlrpc_user_desc *pud; >> >> pud = lustre_msg_buf(msg, offset, 0); >> + if (!pud) >> + return -EINVAL; >> >> pud->pud_uid = from_kuid(&init_user_ns, current_uid()); >> pud->pud_gid = from_kgid(&init_user_ns, current_gid()); >> diff --git a/drivers/staging/lustre/lustre/ptlrpc/sec_plain.c b/drivers/staging/lustre/lustre/ptlrpc/sec_plain.c >> index 37c9f4c..7736aa9 100644 >> --- a/drivers/staging/lustre/lustre/ptlrpc/sec_plain.c >> +++ b/drivers/staging/lustre/lustre/ptlrpc/sec_plain.c >> @@ -542,6 +542,7 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec, >> { >> __u32 buflens[PLAIN_PACK_SEGMENTS] = { 0, }; >> int alloc_len; >> + int desc; >> >> buflens[PLAIN_PACK_HDR_OFF] = sizeof(struct plain_header); >> buflens[PLAIN_PACK_MSG_OFF] = msgsize; >> @@ -575,7 +576,10 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec, >> req->rq_reqmsg = lustre_msg_buf(req->rq_reqbuf, PLAIN_PACK_MSG_OFF, 0); >> >> if (req->rq_pack_udesc) >> - sptlrpc_pack_user_desc(req->rq_reqbuf, PLAIN_PACK_USER_OFF); >> + desc = sptlrpc_pack_user_desc(req->rq_reqbuf, PLAIN_PACK_USER_OFF); > This makes the line longer than 80 chars. > Also why not declare desc right here as opposed to the start of the function like you did? > >> + if (!desc){ >> + return desc; >> + } > We don't really need these curvy braces here. > > Thanks! > > Thanks for the feedback. I'm sending a new patch in now. Lidza