linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] cifs: avoid extra calls in posix_info_parse
@ 2021-06-18  0:58 Ronnie Sahlberg
  2021-06-18  3:54 ` Steve French
  0 siblings, 1 reply; 2+ messages in thread
From: Ronnie Sahlberg @ 2021-06-18  0:58 UTC (permalink / raw)
  To: linux-cifs; +Cc: Steve French

In posix_info_parse() we call posix_info_sid_size twice for each of the owner and the group
sid. The first time to check that it is valid, i.e. >= 0 and the second time
to just pass it in as a length to memcpy().
As this is a pure function we know that it can not be negative the second time and this
is technically a false warning in coverity.
However, as it is a pure function we are just wasting cycles by calling it a second time.
Record the length from the first time we call it and save some cycles as well as make
Coverity happy.

Addresses-Coverity-ID: 1491379 ("Argument can not be negative")

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/smb2pdu.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index c205f93e0a10..4a244cc4e902 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -4498,7 +4498,7 @@ int posix_info_parse(const void *beg, const void *end,
 
 {
 	int total_len = 0;
-	int sid_len;
+	int owner_len, group_len;
 	int name_len;
 	const void *owner_sid;
 	const void *group_sid;
@@ -4521,17 +4521,17 @@ int posix_info_parse(const void *beg, const void *end,
 
 	/* check owner sid */
 	owner_sid = beg + total_len;
-	sid_len = posix_info_sid_size(owner_sid, end);
-	if (sid_len < 0)
+	owner_len = posix_info_sid_size(owner_sid, end);
+	if (owner_len < 0)
 		return -1;
-	total_len += sid_len;
+	total_len += owner_len;
 
 	/* check group sid */
 	group_sid = beg + total_len;
-	sid_len = posix_info_sid_size(group_sid, end);
-	if (sid_len < 0)
+	group_len = posix_info_sid_size(group_sid, end);
+	if (group_len < 0)
 		return -1;
-	total_len += sid_len;
+	total_len += group_len;
 
 	/* check name len */
 	if (beg + total_len + 4 > end)
@@ -4552,10 +4552,8 @@ int posix_info_parse(const void *beg, const void *end,
 		out->size = total_len;
 		out->name_len = name_len;
 		out->name = name;
-		memcpy(&out->owner, owner_sid,
-		       posix_info_sid_size(owner_sid, end));
-		memcpy(&out->group, group_sid,
-		       posix_info_sid_size(group_sid, end));
+		memcpy(&out->owner, owner_sid, owner_len);
+		memcpy(&out->group, group_sid, group_len);
 	}
 	return total_len;
 }
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] cifs: avoid extra calls in posix_info_parse
  2021-06-18  0:58 [PATCH] cifs: avoid extra calls in posix_info_parse Ronnie Sahlberg
@ 2021-06-18  3:54 ` Steve French
  0 siblings, 0 replies; 2+ messages in thread
From: Steve French @ 2021-06-18  3:54 UTC (permalink / raw)
  To: Ronnie Sahlberg; +Cc: linux-cifs

Don't we have a secondary problem (have you been able to pull up a
more complete coverity explanation of the warning - I just see minimal
info on the line numbers of the two functions returning -1)?

e.g. if posix_info_sid_size returns -1, posix_info_parse returns -1
... but we don't check for the rc in one of the callers
cifs_posix_to_fattr

/* Fill a cifs_fattr struct with info from SMB_FIND_FILE_POSIX_INFO. */
static void
cifs_posix_to_fattr(struct cifs_fattr *fattr, struct smb2_posix_info *info,
                    struct cifs_sb_info *cifs_sb)
{
        struct smb2_posix_info_parsed parsed;

        posix_info_parse(info, NULL, &parsed);
... NEED TO CHECK RC HERE ...
        memset(fattr, 0, sizeof(*fattr));
...

On Thu, Jun 17, 2021 at 7:58 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> In posix_info_parse() we call posix_info_sid_size twice for each of the owner and the group
> sid. The first time to check that it is valid, i.e. >= 0 and the second time
> to just pass it in as a length to memcpy().
> As this is a pure function we know that it can not be negative the second time and this
> is technically a false warning in coverity.
> However, as it is a pure function we are just wasting cycles by calling it a second time.
> Record the length from the first time we call it and save some cycles as well as make
> Coverity happy.
>
> Addresses-Coverity-ID: 1491379 ("Argument can not be negative")
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/smb2pdu.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index c205f93e0a10..4a244cc4e902 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -4498,7 +4498,7 @@ int posix_info_parse(const void *beg, const void *end,
>
>  {
>         int total_len = 0;
> -       int sid_len;
> +       int owner_len, group_len;
>         int name_len;
>         const void *owner_sid;
>         const void *group_sid;
> @@ -4521,17 +4521,17 @@ int posix_info_parse(const void *beg, const void *end,
>
>         /* check owner sid */
>         owner_sid = beg + total_len;
> -       sid_len = posix_info_sid_size(owner_sid, end);
> -       if (sid_len < 0)
> +       owner_len = posix_info_sid_size(owner_sid, end);
> +       if (owner_len < 0)
>                 return -1;
> -       total_len += sid_len;
> +       total_len += owner_len;
>
>         /* check group sid */
>         group_sid = beg + total_len;
> -       sid_len = posix_info_sid_size(group_sid, end);
> -       if (sid_len < 0)
> +       group_len = posix_info_sid_size(group_sid, end);
> +       if (group_len < 0)
>                 return -1;
> -       total_len += sid_len;
> +       total_len += group_len;
>
>         /* check name len */
>         if (beg + total_len + 4 > end)
> @@ -4552,10 +4552,8 @@ int posix_info_parse(const void *beg, const void *end,
>                 out->size = total_len;
>                 out->name_len = name_len;
>                 out->name = name;
> -               memcpy(&out->owner, owner_sid,
> -                      posix_info_sid_size(owner_sid, end));
> -               memcpy(&out->group, group_sid,
> -                      posix_info_sid_size(group_sid, end));
> +               memcpy(&out->owner, owner_sid, owner_len);
> +               memcpy(&out->group, group_sid, group_len);
>         }
>         return total_len;
>  }
> --
> 2.30.2
>


-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-06-18  3:55 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-18  0:58 [PATCH] cifs: avoid extra calls in posix_info_parse Ronnie Sahlberg
2021-06-18  3:54 ` Steve French

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).