11:29 a.m.
I’m going to cc: the devel mailing list for more opinions
This code looks (mostly) safe to me. The concern I have is that it isn’t using gsh_malloc
and gsh_free yet I assume that the buffer when freed later is freed with gsh_free.
Frank
From: Paul Moyer [mailto:Paul.Moyer@ibm.com]
Sent: Friday, November 4, 2022 7:55 AM
To: Frank Filz <ffilzlnx(a)mindspring.com>
Subject: RE: [S] Change in ...nfs-ganesha[next]: fixed coverity issue 275287 (dead code)
by ifdefing out the unused code
After looking at this closer I don’t think it is a problem:
In function ‘xdr_utf8string_decode’,
inlined from ‘inline_xdr_utf8string’ at
/home/moyer/275287/nfs-ganesha/src/include/nfsv41.h:4055:3,
inlined from ‘decode_acl’ at
/home/moyer/275287/nfs-ganesha/src/Protocols/NFS/nfs_proto_tools.c:834:29:
/home/moyer/275287/nfs-ganesha/src/include/nfsv41.h:4041:8: warning: attempt to free a
non-heap object ‘buffer’ [-Wfree-nonheap-object]
free(sp);
^
looking at the code (snipets)
3991 static inline bool
3992 xdr_utf8string_decode(XDR *xdrs, utf8string *objp, u_int maxsize)
3993 {
3994 /* sp is the actual string pointer */
3995 char *sp = objp->utf8string_val;
4026 if (!sp) {
4027 sp = (char *)malloc(size + 1);
4036 ret = xdr_opaque_decode(xdrs, sp, size);
4038 if (!ret) {
4039 if (!objp->utf8string_val) {
4040 /* Only free if we allocated */
4041 free(sp);
4042 }
4043 return ret;
4044 }
4045
4046 objp->utf8string_val = sp; /* only valid po
inter */
so the flow seems to be:
sp is a pointer assigned to a string inside obj which is passed as a parameter
if sp is null, malloc is called and sp now points to “locally” allocated space
if an error occurs in xdr_opaque_decode(), enter the error path starting at 4038
if the string (in the passed object) that sp was originally assigned to is null, sp is
free'd
therefore, sp is only malloc'ed if the original string is null, and if the original
string is null, sp is free'd
which is consistent with the comment on 4040, though the logic took some study ...
the pointer in the passed object is subsequently replaced with sp:
if the pointer was not null to begin with, sp would already point to the same string
if the pointer was null, memory would be allocated and the passed object would be updated
with a pointer to the “locally” allocated memory
i think this works because this method is declared as "static inline", so the
allocated memory is ultimately local to the caller anyway ?
Let me know what you think.
From: Frank Filz <ffilzlnx(a)mindspring.com <mailto:ffilzlnx@mindspring.com> >
Sent: Thursday, November 3, 2022 3:07 PM
To: Paul Moyer <Paul.Moyer(a)ibm.com <mailto:Paul.Moyer@ibm.com> >
Subject: [EXTERNAL] RE: [S] Change in ...nfs-ganesha[next]: fixed coverity issue 275287
(dead code) by ifdefing out the unused code
Hmm, I’ve never seen that warning. And I didn’t realize we had code that did malloc/free…
We should investigate that more closely. Frank From: Paul Moyer
[mailto: Paul. Moyer@ ibm. com]
ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender
This message came from outside your organization.
ZjQcmQRYFpfptBannerEnd
Hmm, I’ve never seen that warning.
And I didn’t realize we had code that did malloc/free…
We should investigate that more closely.
Frank
From: Paul Moyer [mailto:Paul.Moyer@ibm.com]
Sent: Thursday, November 3, 2022 11:19 AM
To: ffilzlnx(a)mindspring.com <mailto:ffilzlnx@mindspring.com>
Subject: RE: [S] Change in ...nfs-ganesha[next]: fixed coverity issue 275287 (dead code)
by ifdefing out the unused code
Sorry for the email, but I have yet to figure out IRC.
While building the fix for 275287 I happened to notice a compiler warning:
[ 43%] Building C object Protocols/NFS/CMakeFiles/nfsproto.dir/nfs_proto_tools.c.o
In file included from /home/moyer/275287/nfs-ganesha/src/include/fsal_types.h:44:0,
from /home/moyer/275287/nfs-ganesha/src/include/fsal_api.h:45,
from /home/moyer/275287/nfs-ganesha/src/include/fsal.h:63,
from
/home/moyer/275287/nfs-ganesha/src/Protocols/NFS/nfs_proto_tools.c:35:
In function ‘xdr_utf8string_decode’,
inlined from ‘inline_xdr_utf8string’ at
/home/moyer/275287/nfs-ganesha/src/include/nfsv41.h:4055:3,
inlined from ‘decode_acl’ at
/home/moyer/275287/nfs-ganesha/src/Protocols/NFS/nfs_proto_tools.c:834:29:
/home/moyer/275287/nfs-ganesha/src/include/nfsv41.h:4041:8: warning: attempt to free a
non-heap object ‘buffer’ [-Wfree-nonheap-object]
free(sp);
^
There is even a comment in the code
4040 /* Only free if we allocated */
4041 free(sp);
Which makes me think someone else may have seen this before but decided not to fix it.
Anyway, should I fix this warning with 275287 ?
From: Frank Filz (GerritHub) <gerrit(a)gerrithub.io <mailto:gerrit@gerrithub.io>
>
Sent: Thursday, November 3, 2022 1:29 PM
To: Paul Moyer <Paul.Moyer(a)ibm.com <mailto:Paul.Moyer@ibm.com> >
Cc: Daniel Gryniewicz <dang(a)redhat.com <mailto:dang@redhat.com> >;
yogendra858(a)yahoo.com <mailto:yogendra858@yahoo.com> ; alokchandra.mallick(a)gmail.com
<mailto:alokchandra.mallick@gmail.com> ; Gluster Community Jenkins
<gerrithub(a)gluster.org <mailto:gerrithub@gluster.org> >
Subject: [EXTERNAL] [S] Change in ...nfs-ganesha[next]: fixed coverity issue 275287 (dead
code) by ifdefing out the unused code
Attention is currently required from: Daniel Gryniewicz, yogendra858(a) yahoo. com,
alokchandra. mallick(a) gmail. com, Paul Moyer. Patch set 1: Code-Review -1 View Change 1
comment: File src/SAL/recovery/recovery_fs_ng. c: Patch Set #1, Line 371:
ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender
This message came from outside your organization.
ZjQcmQRYFpfptBannerEnd
Attention is currently required from: Daniel Gryniewicz, yogendra858(a)yahoo.com
<mailto:yogendra858@yahoo.com> , alokchandra.mallick(a)gmail.com
<mailto:alokchandra.mallick@gmail.com> , Paul Moyer.
Patch set 1:Code-Review -1
View Change <https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/545670>
1 comment:
* File src/SAL/recovery/recovery_fs_ng.c:
* Patch Set #1, Line 371:
<https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/545670/comment/4e74c97b...
{
oops, need to ifdef out rc...
To view, visit change 545670
<https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/545670> . To unsubscribe, or for
help writing mail filters, visit settings <https://review.gerrithub.io/settings> .
Gerrit-Project: ffilz/nfs-ganesha
Gerrit-Branch: next
Gerrit-Change-Id: I1fa6854136986774cc8f9cb87371f5709cbef935
Gerrit-Change-Number: 545670
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Moyer <paul.moyer(a)ibm.com <mailto:paul.moyer@ibm.com> >
Gerrit-Reviewer: Daniel Gryniewicz <dang(a)redhat.com <mailto:dang@redhat.com> >
Gerrit-Reviewer: Frank Filz <ffilzlnx(a)mindspring.com
<mailto:ffilzlnx@mindspring.com> >
Gerrit-Reviewer: Gluster Community Jenkins <gerrithub(a)gluster.org
<mailto:gerrithub@gluster.org> >
Gerrit-Reviewer: alokchandra.mallick(a)gmail.com
<mailto:alokchandra.mallick@gmail.com>
Gerrit-Reviewer: yogendra858(a)yahoo.com <mailto:yogendra858@yahoo.com>
Gerrit-Attention: Daniel Gryniewicz <dang(a)redhat.com <mailto:dang@redhat.com>
>
Gerrit-Attention: yogendra858(a)yahoo.com <mailto:yogendra858@yahoo.com>
Gerrit-Attention: alokchandra.mallick(a)gmail.com
<mailto:alokchandra.mallick@gmail.com>
Gerrit-Attention: Paul Moyer <paul.moyer(a)ibm.com <mailto:paul.moyer@ibm.com> >
Gerrit-Comment-Date: Thu, 03 Nov 2022 17:29:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment