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@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@mindspring.com>
Sent: Thursday, November 3, 2022 3:07 PM
To: Paul Moyer <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@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@gerrithub.io>
Sent: Thursday, November 3, 2022 1:29 PM
To: Paul Moyer <Paul.Moyer@ibm.com>
Cc: Daniel Gryniewicz <dang@redhat.com>; yogendra858@yahoo.com; alokchandra.mallick@gmail.com; Gluster Community Jenkins <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@ yahoo. com, alokchandra. mallick@ 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@yahoo.com, alokchandra.mallick@gmail.com, Paul Moyer.

Patch set 1:Code-Review -1

View Change

1 comment: