Fix memory leak in glfs_h_create_from_handle

[ThalesBarretto]

Fix memory leak in glfs_h_create_from_handle

If the allocation of the glfs_object failed, the reference to the newly found or linked inode was leaked. We introduce a call to inode_unref(newinode) guarded by a check, so that the newinode does not leak in such case. One of the paths also required a solution for the
inode_lookup case, which we also introduce.

The leak happened in two cases:

1. The inode_new Path (New Inode)

loc.inode = inode_new(): new inode, first reference (refcount = 1) owned by loc.inode.
newinode = inode_link(loc.inode, ...):
inode_link calls __inode_link -> hashes loc.inode and returns it.
inode_link then calls __inode_ref() -> inode has refcount = 2.
loc.inode <- first reference, and newinode <- second (new) reference.

2. The lookup_needed Path (Existing Inode)]

newinode = inode_find(): This finds the inode, takes reference (refcount = 1).
loc.inode = newinode: No new reference; both share the single reference.
newinode = inode_link(loc.inode, ...)
Since the inode is already hashed, inode_link finds it and returns it.
It then calls __inode_ref(), taking a second reference (refcount = 2).
newinode is updated to hold this second reference, while loc.inode effectively "keeps" the original reference from inode_find.

In both scenarios, after inode_link you have two references to the inode.

And then we lookup the inode, increasing it's nlookup reference count:

inode_lookup(new_inode): increments new_inode->nlookup

Forward in the successful path, object->inode = newinode takes
ownership of one reference, and loc_wipe(&loc) correctly drops the temporary
reference held by loc.inode, leaving exactly one reference owned by the
glfs_object.

BUT if GF_CALLOC fails, you must call inode_unref(newinode) to drop one
reference, and then loc_wipe(&loc) handles the other. In this case, inode_unref
decrements ref, and when it hits zero, the fate of the inode depends on nlookup.
Since we inode_lookup'd, inode->nlookup > 0 and we go __inode_passivate() when
we expected __inode_retire() (inode.c:521-528).

That's why we introduce the variable, the check and the call to
decrement the nlookup reference count in the GF_CALLOC fail path before
calling inode_unref.

This closes the TODO at the 'out:' label of glfs_h_create_from_handle.

[pranithk]

loc_wipe(&loc) should free this inode right?

[ThalesBarretto]

1. The inode_new Path (New Inode)
   loc.inode = inode_new(): new inode, first reference (refcount = 1) owned by loc.inode.
   newinode = inode_link(loc.inode, ...):
   inode_link calls __inode_link -> hashes loc.inode and returns it.
   inode_link then calls __inode_ref() -> inode has refcount = 2.
   loc.inode <- first reference, and newinode <- second (new) reference.

2. The lookup_needed Path (Existing Inode)
   newinode = inode_find(...): This finds the inode, takes reference (refcount = 1).
   loc.inode = newinode: No new reference; both share the single reference.
   newinode = inode_link(loc.inode, ...):
   * Since the inode is already hashed, inode_link finds it and returns it.
   * It then calls __inode_ref(), taking a second reference (refcount = 2).
   * newinode is updated to hold this second reference, while loc.inode effectively "keeps" the original reference from inode_find.

In both scenarios, after inode_link you have two references to the inode.

In the success path, object->inode = newinode takes ownership of one reference, and loc_wipe(&loc) correctly drops the temporary reference held by loc.inode, leaving exactly one reference owned by the glfs_object.

BUT if GF_CALLOC fails, you must call inode_unref(newinode) to drop one reference, and then loc_wipe(&loc) handles the other.

[pranithk]

You need to also handle this extra lookup that is happening. This is probably what you have to do.

pranith.karampuri@PP-40RQS54 - ~/workspace/glusterfs (devel)
09:43:49 :) ⚡ git diff
diff --git a/api/src/glfs-handleops.c b/api/src/glfs-handleops.c
index d5bdb54ef9..d3e0d0759f 100644
--- a/api/src/glfs-handleops.c
+++ b/api/src/glfs-handleops.c
@@ -1415,6 +1415,7 @@ pub_glfs_h_create_from_handle(struct glfs *fs, unsigned char *handle, int len,
     struct glfs_object *object = NULL;
     uint64_t ctx_value = LOOKUP_NOT_NEEDED;
     gf_boolean_t lookup_needed = _gf_false;
+    gf_boolean_t inode_looked_up = _gf_false;
 
     /* validate in args */
     if ((fs == NULL) || (handle == NULL) || (len != GFAPI_HANDLE_LENGTH)) {
@@ -1487,6 +1488,7 @@ pub_glfs_h_create_from_handle(struct glfs *fs, unsigned char *handle, int len,
             inode_ctx_set(newinode, THIS, &ctx_value);
         }
         inode_lookup(newinode);
+        inode_looked_up = _gf_true;
     } else {
         gf_smsg(subvol->name, GF_LOG_WARNING, errno, API_MSG_INODE_LINK_FAILED,
                 "gfid=%s", uuid_utoa(loc.gfid), NULL);
@@ -1502,6 +1504,10 @@ found:
     if (object == NULL) {
         errno = ENOMEM;
         ret = -1;
+        if (inode_looked_up) {
+            inode_forget(newinode, 1);
+        }
+        inode_unref(newinode);
         goto out;
     }

[ThalesBarretto]

agreed, will update the patch

[ThalesBarretto]

1. The inode_new Path (New Inode)
   loc.inode = inode_new(): new inode, first reference (refcount = 1) owned by loc.inode.
   newinode = inode_link(loc.inode, ...):
   inode_link calls __inode_link -> hashes loc.inode and returns it.
   inode_link then calls __inode_ref() -> inode has refcount = 2.
   loc.inode <- first reference, and newinode <- second (new) reference.

2. The lookup_needed Path (Existing Inode)
   newinode = inode_find(...): This finds the inode, takes reference (refcount = 1).
   loc.inode = newinode: No new reference; both share the single reference.
   newinode = inode_link(loc.inode, ...):
   * Since the inode is already hashed, inode_link finds it and returns it.
   * It then calls __inode_ref(), taking a second reference (refcount = 2).
   * newinode is updated to hold this second reference, while loc.inode effectively "keeps" the original reference from inode_find.

In both scenarios, after inode_link you have two references to the inode.

In the success path, object->inode = newinode takes ownership of one reference, and loc_wipe(&loc) correctly drops the temporary reference held by loc.inode, leaving exactly one reference owned by the glfs_object.

BUT if GF_CALLOC fails, you must call inode_unref(newinode) to drop one reference, and then loc_wipe(&loc) handles the other.

[ThalesBarretto]

agreed, will update the patch