livepatch: Reorder to use before freeing a pointer

PATCH In review [email protected] 4 participants 7 messages Mar 20, 2022 – Mar 23, 2022 Live patching

lore

Share & tools

lore

[email protected]

author

2022-03-20 01:51 UTC #0 lore raw

PATCH v2 1/1 livepatch: Reorder to use before freeing a pointer

From: Tom Rix <[email protected]>

Clang static analysis reports this issue livepatch-shadow-fix1.c:113:2: warning: Use of
memory after it is freed
pr_info("%s: dummy @ %p, prevented leak @ %p\n", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The pointer is freed in the previous statement.
Reorder the pr_info to report before the free.

Similar issue in livepatch-shadow-fix2.c

Signed-off-by Tom Rix <[email protected]>

---
v2: Fix similar issue in livepatch-shadow-fix2.c

 samples/livepatch/livepatch-shadow-fix1.c | 2 +-
 samples/livepatch/livepatch-shadow-fix2.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

samples/livepatch/livepatch-shadow-fix1.c +1 -1

index 918ce17b43fda..6701641bf12d4 100644
@@ -109,9 +109,9 @@ static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data)
 	void *d = obj;
 	int **shadow_leak = shadow_data;
 
-	kfree(*shadow_leak);
 	pr_info("%s: dummy @ %p, prevented leak @ %p\n",
 			 __func__, d, *shadow_leak);
+	kfree(*shadow_leak);
 }
 
 static void livepatch_fix1_dummy_free(struct dummy *d)

samples/livepatch/livepatch-shadow-fix2.c +1 -1

index 29fe5cd420472..361046a4f10cf 100644
@@ -61,9 +61,9 @@ static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data)
 	void *d = obj;
 	int **shadow_leak = shadow_data;
 
-	kfree(*shadow_leak);
 	pr_info("%s: dummy @ %p, prevented leak @ %p\n",
 			 __func__, d, *shadow_leak);
+	kfree(*shadow_leak);
 }
 
 static void livepatch_fix2_dummy_free(struct dummy *d)

Signature · 1 line

2.26.3

Petr Mladek

maintainer Reviewed-by on v2 1/1

2022-03-21 13:24 UTC #1 lore raw

Quoted 8 lines · On Sat 2022-03-19 18:51:43, [email protected] wrote:

From: Tom Rix <[email protected]>
Clang static analysis reports this issue livepatch-shadow-fix1.c:113:2: warning: Use of
memory after it is freed
pr_info("%s: dummy @ %p, prevented leak @ %p\n", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Quoted 6 lines

The pointer is freed in the previous statement.
Reorder the pr_info to report before the free.
Similar issue in livepatch-shadow-fix2.c
Signed-off-by Tom Rix <[email protected]>

Strictly speaking, the freed memory is not used.
pr_info() only prints the address.

Anyway, I agree that the reordered code is cleaner.

Reviewed-by Petr Mladek <[email protected]>

Best Regards,
Petr

Joe Lawrence

maintainer Acked-by on v2 1/1

2022-03-21 13:39 UTC #2 lore raw

In reply to [email protected]

Quoted 42 lines · On 3/19/22 9:51 PM, [email protected] wrote:

From: Tom Rix <[email protected]>
Clang static analysis reports this issue livepatch-shadow-fix1.c:113:2: warning: Use of
memory after it is freed
pr_info("%s: dummy @ %p, prevented leak @ %p\n", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The pointer is freed in the previous statement.
Reorder the pr_info to report before the free.
Similar issue in livepatch-shadow-fix2.c
Signed-off-by Tom Rix <[email protected]>
---
v2: Fix similar issue in livepatch-shadow-fix2.c
 samples/livepatch/livepatch-shadow-fix1.c | 2 +-
 samples/livepatch/livepatch-shadow-fix2.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
samples/livepatch/livepatch-shadow-fix1.c +1 -1
index 918ce17b43fda..6701641bf12d4 100644
@@ -109,9 +109,9 @@ static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data)
 	void *d = obj;
 	int **shadow_leak = shadow_data;
 
-	kfree(*shadow_leak);
 	pr_info("%s: dummy @ %p, prevented leak @ %p\n",
 			 __func__, d, *shadow_leak);
+	kfree(*shadow_leak);
 }
 
 static void livepatch_fix1_dummy_free(struct dummy *d)
samples/livepatch/livepatch-shadow-fix2.c +0 -0
index 29fe5cd420472..361046a4f10cf 100644

samples/livepatch/livepatch-shadow-fix2.c static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data) · line 69

Quoted 10 lines

samples/livepatch/livepatch-shadow-fix2.c +1 -1

@@ -61,9 +61,9 @@ static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data)
 	void *d = obj;
 	int **shadow_leak = shadow_data;
 
-	kfree(*shadow_leak);
 	pr_info("%s: dummy @ %p, prevented leak @ %p\n",
 			 __func__, d, *shadow_leak);
+	kfree(*shadow_leak);
 }
 
 static void livepatch_fix2_dummy_free(struct dummy *d)

Hi Tom,

Ordering doesn't matter for the example, so let's clean up the static analysis.

Acked-by Joe Lawrence <[email protected]>

But for my sanity, isn't this a false positive? There shouldn't be harm in printing the pointer itself, even after what it points to has been freed, i.e.

	int *i = malloc(sizeof(*i));
	free(i);
	printf("%p\n", i);      << ok
	printf("%d\n", *i);     << NOT ok

But I suppose clang doesn't know that the passed pointer isn't getting dereferenced by the function, so it throws up a warning? Just curious what your experience has been with respect to these reports.

Thanks,

Signature · 1 line

Joe

Tom Rix

author

2022-03-21 14:06 UTC #3 lore raw

Quoted 64 lines · Joe Lawrence

Quoted 42 lines · On 3/19/22 9:51 PM, [email protected] wrote:
From: Tom Rix <[email protected]>
Clang static analysis reports this issue livepatch-shadow-fix1.c:113:2: warning: Use of
memory after it is freed
pr_info("%s: dummy @ %p, prevented leak @ %p\n", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The pointer is freed in the previous statement.
Reorder the pr_info to report before the free.
Similar issue in livepatch-shadow-fix2.c
Signed-off-by Tom Rix <[email protected]>
---
v2: Fix similar issue in livepatch-shadow-fix2.c
  samples/livepatch/livepatch-shadow-fix1.c | 2 +-
  samples/livepatch/livepatch-shadow-fix2.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)
samples/livepatch/livepatch-shadow-fix1.c +1 -1
index 918ce17b43fda..6701641bf12d4 100644
@@ -109,9 +109,9 @@ static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data)
  	void *d = obj;
  	int **shadow_leak = shadow_data;
 
-	kfree(*shadow_leak);
  	pr_info("%s: dummy @ %p, prevented leak @ %p\n",
  			 __func__, d, *shadow_leak);
+	kfree(*shadow_leak);
  }
 
  static void livepatch_fix1_dummy_free(struct dummy *d)
samples/livepatch/livepatch-shadow-fix2.c +0 -0
index 29fe5cd420472..361046a4f10cf 100644
samples/livepatch/livepatch-shadow-fix2.c static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data) · line 69
Quoted 10 lines
samples/livepatch/livepatch-shadow-fix2.c +1 -1
@@ -61,9 +61,9 @@ static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data)
  	void *d = obj;
  	int **shadow_leak = shadow_data;
 
-	kfree(*shadow_leak);
  	pr_info("%s: dummy @ %p, prevented leak @ %p\n",
  			 __func__, d, *shadow_leak);
+	kfree(*shadow_leak);
  }
 
  static void livepatch_fix2_dummy_free(struct dummy *d)
Hi Tom,
Ordering doesn't matter for the example, so let's clean up the static analysis.
Acked-by Joe Lawrence <[email protected]>
But for my sanity, isn't this a false positive? There shouldn't be harm in printing the pointer itself, even after what it points to has been freed, i.e.

Quoted 8 lines

	int *i = malloc(sizeof(*i));
	free(i);
	printf("%p\n", i);      << ok
	printf("%d\n", *i);     << NOT ok
But I suppose clang doesn't know that the passed pointer isn't getting dereferenced by the function, so it throws up a warning? Just curious what your experience has been with respect to these reports.

The analysis it good for static functions, for extern functions it has nothing to analyze so a worst case is assumed.

I agree this is likely a false positive.

Tom

Quoted 2 lines

Thanks,

David Vernet

Acked-by

2022-03-23 03:21 UTC #4 lore raw

Quoted 40 lines

Quoted 40 lines · On 3/19/22 9:51 PM, [email protected] wrote:
Quoted 39 lines
From: Tom Rix <[email protected]>
Clang static analysis reports this issue livepatch-shadow-fix1.c:113:2: warning: Use of
memory after it is freed
pr_info("%s: dummy @ %p, prevented leak @ %p\n", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The pointer is freed in the previous statement.
Reorder the pr_info to report before the free.
Similar issue in livepatch-shadow-fix2.c
Signed-off-by Tom Rix <[email protected]>
---
v2: Fix similar issue in livepatch-shadow-fix2.c
  samples/livepatch/livepatch-shadow-fix1.c | 2 +-
  samples/livepatch/livepatch-shadow-fix2.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)
samples/livepatch/livepatch-shadow-fix1.c +1 -1
index 918ce17b43fda..6701641bf12d4 100644
@@ -109,9 +109,9 @@ static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data)
  	void *d = obj;
  	int **shadow_leak = shadow_data;
-	kfree(*shadow_leak);
  	pr_info("%s: dummy @ %p, prevented leak @ %p\n",
  			 __func__, d, *shadow_leak);
+	kfree(*shadow_leak);
  }
  static void livepatch_fix1_dummy_free(struct dummy *d)
samples/livepatch/livepatch-shadow-fix2.c +0 -0
index 29fe5cd420472..361046a4f10cf 100644
@@ -61,9 +61,9 @@ static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data)

Quoted 8 lines

Quoted 8 lines
samples/livepatch/livepatch-shadow-fix2.c static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data) · line 69
Quoted 8 lines
samples/livepatch/livepatch-shadow-fix2.c +1 -1
  	void *d = obj;
  	int **shadow_leak = shadow_data;
-	kfree(*shadow_leak);
  	pr_info("%s: dummy @ %p, prevented leak @ %p\n",
  			 __func__, d, *shadow_leak);
+	kfree(*shadow_leak);
  }
  static void livepatch_fix2_dummy_free(struct dummy *d)

Acked-by David Vernet <[email protected]>

Petr Mladek

maintainer

2022-03-23 15:53 UTC #5 lore raw

In reply to [email protected]

Quoted 8 lines · On Sat 2022-03-19 18:51:43, [email protected] wrote:

From: Tom Rix <[email protected]>
Clang static analysis reports this issue livepatch-shadow-fix1.c:113:2: warning: Use of
memory after it is freed
pr_info("%s: dummy @ %p, prevented leak @ %p\n", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Quoted 4 lines

The pointer is freed in the previous statement.
Reorder the pr_info to report before the free.
Similar issue in livepatch-shadow-fix2.c

I have added the following paragraph:

<snip>
Note that it is a false positive. pr_info() just prints the address. The freed memory is not accessed. Well, the static analyzer could not know this easily.
</snip>

Quoted 1 line

Signed-off-by Tom Rix <[email protected]>

and pushed the patch into livepatching/livepatching.git, branch for-5.18/selftests-fixes.

IMHO, the patch is so trivial and can be added even in this merge window. There is no need to create more dances around it ;-)

Let me know if you disagree. I am going to send the pull request on Friday or Monday.

Best Regards,
Petr

Tom Rix

author

2022-03-23 16:17 UTC #6 lore raw

Quoted 22 lines · Petr Mladek

Quoted 8 lines · On Sat 2022-03-19 18:51:43, [email protected] wrote:
From: Tom Rix <[email protected]>
Clang static analysis reports this issue livepatch-shadow-fix1.c:113:2: warning: Use of
memory after it is freed
pr_info("%s: dummy @ %p, prevented leak @ %p\n", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Quoted 4 lines
The pointer is freed in the previous statement.
Reorder the pr_info to report before the free.
Similar issue in livepatch-shadow-fix2.c
I have added the following paragraph:
<snip>
Note that it is a false positive. pr_info() just prints the address. The freed memory is not accessed. Well, the static analyzer could not know this easily.
</snip>
Quoted 1 line
Signed-off-by Tom Rix <[email protected]>

Quoted 8 lines

and pushed the patch into livepatching/livepatching.git, branch for-5.18/selftests-fixes.
IMHO, the patch is so trivial and can be added even in this merge window. There is no need to create more dances around it ;-)
Let me know if you disagree. I am going to send the pull request on Friday or Monday.

Do whatever is easier for you. The addition to the commit log is fine.

Thanks

Tom

Quoted 4 lines

Best Regards,
Petr