Skip to content

Commit 76d7adc

Browse files
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) <- 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 in label 'out:' of glfs_h_create_from_handle Signed-off-by: Thales Antunes de Oliveira Barretto <thales.barretto.git@gmail.com>
1 parent ae1d696 commit 76d7adc

1 file changed

Lines changed: 6 additions & 1 deletion

File tree

api/src/glfs-handleops.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1415,6 +1415,7 @@ pub_glfs_h_create_from_handle(struct glfs *fs, unsigned char *handle, int len,
14151415
struct glfs_object *object = NULL;
14161416
uint64_t ctx_value = LOOKUP_NOT_NEEDED;
14171417
gf_boolean_t lookup_needed = _gf_false;
1418+
gf_boolean_t inode_looked_up = _gf_false;
14181419

14191420
/* validate in args */
14201421
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,
14871488
inode_ctx_set(newinode, THIS, &ctx_value);
14881489
}
14891490
inode_lookup(newinode);
1491+
inode_looked_up = _gf_true;
14901492
} else {
14911493
gf_smsg(subvol->name, GF_LOG_WARNING, errno, API_MSG_INODE_LINK_FAILED,
14921494
"gfid=%s", uuid_utoa(loc.gfid), NULL);
@@ -1502,6 +1504,10 @@ pub_glfs_h_create_from_handle(struct glfs *fs, unsigned char *handle, int len,
15021504
if (object == NULL) {
15031505
errno = ENOMEM;
15041506
ret = -1;
1507+
if (inode_looked_up) {
1508+
inode_forget(newinode, 1);
1509+
}
1510+
inode_unref(newinode);
15051511
goto out;
15061512
}
15071513

@@ -1510,7 +1516,6 @@ pub_glfs_h_create_from_handle(struct glfs *fs, unsigned char *handle, int len,
15101516
gf_uuid_copy(object->gfid, object->inode->gfid);
15111517

15121518
out:
1513-
/* TODO: Check where the inode ref is being held? */
15141519
loc_wipe(&loc);
15151520

15161521
glfs_subvol_done(fs, subvol);

0 commit comments

Comments
 (0)