fs/ext3: set pointer = NULL, after kfree it

PATCH In review Chen Gang 3 participants 4 messages Dec 24, 2012 – Dec 31, 2012

lore

Share & tools

lore

Chen Gang

author

2012-12-24 05:57 UTC #0 lore raw

PATCH 1/1 fs/ext3: set pointer = NULL, after kfree it

set s->base = NULL, after kfree it.

Signed-off-by Chen Gang <[email protected]>

---
 fs/ext3/xattr.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/ext3/xattr.c +3 -1

index d22ebb7..57aab94 100644
@@ -847,8 +847,10 @@ cleanup:
 	if (ce)
 		mb_cache_entry_release(ce);
 	brelse(new_bh);
-	if (!(bs->bh && s->base == bs->bh->b_data))
+	if (!(bs->bh && s->base == bs->bh->b_data)) {
 		kfree(s->base);
+		s->base = NULL;
+	}
 
 	return error;

Signature · 1 line

1.7.10.4

Theodore Ts'o

2012-12-25 18:52 UTC #1 lore raw

Quoted 4 lines · Chen Gang

set s->base = NULL, after kfree it.
Signed-off-by Chen Gang <[email protected]>

Was this to fix something flagged by some static code checker? The only caller of ext3_xattr_block_set() is ext3_xattr_set_handle(), and s->base is stored on the stack, and as soon as ext3_xattr_block_set() returns, the object will disappear. So it seems pretty hard to see how this could lead to a use-after-free bug.

- Ted

Chen Gang

author

2012-12-26 02:32 UTC #2 lore raw

Quoted 6 lines · Theodore Ts'o

Quoted 4 lines · Chen Gang
set s->base = NULL, after kfree it.
Signed-off-by Chen Gang <[email protected]>

Quoted 9 lines

Was this to fix something flagged by some static code checker? The only caller of ext3_xattr_block_set() is ext3_xattr_set_handle(), and s->base is stored on the stack, and as soon as ext3_xattr_block_set() returns, the object will disappear. So it seems pretty hard to see how this could lead to a use-after-free bug.
- Ted

this patch is not for a bug

it is only for coding habit:
it is better to set is to NULL, after kfree it.
in the future, if another functions need call ext3_xattr_block_set, too.
it will avoid another members to make mistake.

since it is not for fixing bug, it is really a minor patch. I can understand if you do not apply it.

Signature · 7 lines

Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jan Kara

2012-12-31 15:19 UTC #3 lore raw

Quoted 17 lines · Chen Gang

Quoted 6 lines · Theodore Ts'o
Quoted 4 lines · Chen Gang
set s->base = NULL, after kfree it.
Signed-off-by Chen Gang <[email protected]>
Quoted 9 lines
Was this to fix something flagged by some static code checker? The only caller of ext3_xattr_block_set() is ext3_xattr_set_handle(), and s->base is stored on the stack, and as soon as ext3_xattr_block_set() returns, the object will disappear. So it seems pretty hard to see how this could lead to a use-after-free bug.
- Ted

Quoted 6 lines

this patch is not for a bug
it is only for coding habit:
it is better to set is to NULL, after kfree it.
in the future, if another functions need call ext3_xattr_block_set, too.
it will avoid another members to make mistake.

Well, we set pointers to NULL after kfree() only if there's a good chance
we ever look at the pointer again. This is not the case so I don't think this change makes the code any more safer / easier to read.

Honza

Signature · 6 lines

Jan Kara <[email protected]>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html