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&gt; 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