Skip to contentSkip to conversation

kasan: Replace strreplace() with strchrnul()

PATCH In review Andy Shevchenko 4 participants 7 messages Jun 28, 2023 – Jul 19, 2023

Subsystems (2)
Memory management KASAN
lore
Share & tools
lore

Andy Shevchenko

author
#0 lore raw

PATCH 1/1 kasan: Replace strreplace() with strchrnul()

We don't need to traverse over the entire string and replace occurrences of a character with '\0'. The first match will suffice. Hence, replace strreplace() with strchrnul().

Signed-off-by Andy Shevchenko <[email protected]>
---
 mm/kasan/report_generic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
mm/kasan/report_generic.c +2 -1
index 51a1e8a8877f..63a34eac4a8c 100644
@@ -264,6 +264,7 @@ static void print_decoded_frame_descr(const char *frame_descr)
 	while (num_objects--) {
 		unsigned long offset;
 		unsigned long size;
+		char *p;
 
 		/* access offset */
 		if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),

@@ -282,7 +283,7 @@ static void print_decoded_frame_descr(const char *frame_descr)
 			return;
 
 		/* Strip line number; without filename it's not very helpful. */
-		strreplace(token, ':', '\0');
+		p[strchrnul(token, ':') - token] = '\0';
 
 		/* Finally, print object information. */
 		pr_err(" [%lu, %lu) '%s'", offset, offset + size, token);
Signature · 1 line
2.40.0.1.gaa8946217a0b
reviewer
#1 lore raw
Quoted 23 lines · Andy Shevchenko

We don't need to traverse over the entire string and replace occurrences of a character with '\0'. The first match will suffice. Hence, replace strreplace() with strchrnul().

Signed-off-by Andy Shevchenko <[email protected]>
---
 mm/kasan/report_generic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
mm/kasan/report_generic.c +1 -0
index 51a1e8a8877f..63a34eac4a8c 100644
@@ -264,6 +264,7 @@ static void print_decoded_frame_descr(const char *frame_descr)
        while (num_objects--) {
                unsigned long offset;
                unsigned long size;
+               char *p;
 
                /* access offset */
                if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),

mm/kasan/report_generic.c static void print_decoded_frame_descr(const char *frame_descr) · line 286

Quoted 5 lines
mm/kasan/report_generic.c +1 -1
@@ -282,7 +283,7 @@ static void print_decoded_frame_descr(const char *frame_descr)
                        return;
 
                /* Strip line number; without filename it's not very helpful. */
-               strreplace(token, ':', '\0');
+               p[strchrnul(token, ':') - token] = '\0';

Why not just
*(strchrnul(token, ':')) = '\0';
?

author
#2 lore raw
Quoted 2 lines · Alexander Potapenko

On Wed, Jun 28, 2023 at 5:34 PM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 7 lines

mm/kasan/report_generic.c static void print_decoded_frame_descr(const char *frame_descr) · line 286

Quoted 3 lines
                /* Strip line number; without filename it's not very helpful. */
mm/kasan/report_generic.c +1 -1
-               strreplace(token, ':', '\0');
+               p[strchrnul(token, ':') - token] = '\0';

Why not just
*(strchrnul(token, ':')) = '\0';
?

I don't like Pythonish style in the C. But if you insist, I can update it.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#3 lore raw
In reply to Andy Shevchenko

From: Andy Shevchenko

Quoted 24 lines

Sent: 28 June 2023 16:34

We don't need to traverse over the entire string and replace occurrences of a character with '\0'. The first match will suffice. Hence, replace strreplace() with strchrnul().

Signed-off-by Andy Shevchenko <[email protected]>
---
 mm/kasan/report_generic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
mm/kasan/report_generic.c +1 -0
index 51a1e8a8877f..63a34eac4a8c 100644
@@ -264,6 +264,7 @@ static void print_decoded_frame_descr(const char *frame_descr)
 	while (num_objects--) {
 		unsigned long offset;
 		unsigned long size;
+		char *p;
 
 		/* access offset */
 		if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),

mm/kasan/report_generic.c static void print_decoded_frame_descr(const char *frame_descr) · line 286

Quoted 5 lines
mm/kasan/report_generic.c +1 -1
@@ -282,7 +283,7 @@ static void print_decoded_frame_descr(const char *frame_descr)
 			return;
 
 		/* Strip line number; without filename it's not very helpful. */
-		strreplace(token, ':', '\0');
+		p[strchrnul(token, ':') - token] = '\0';

Isn't 'p' undefined here?

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

author
#4 lore raw
Quoted 2 lines · David Laight

From: Andy Shevchenko

Quoted 1 line

Sent: 28 June 2023 16:34

...

Quoted 5 lines

mm/kasan/report_generic.c static void print_decoded_frame_descr(const char *frame_descr) · line 286

Quoted 3 lines
mm/kasan/report_generic.c +1 -1
 		/* Strip line number; without filename it's not very helpful. */
-		strreplace(token, ':', '\0');
+		p[strchrnul(token, ':') - token] = '\0';

Isn't 'p' undefined here?

Yep, should be token. Not sure what I was thinking about...

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#5 lore raw
In reply to Andy Shevchenko
Quoted 3 lines · Andy Shevchenko

We don't need to traverse over the entire string and replace occurrences of a character with '\0'. The first match will suffice. Hence, replace strreplace() with strchrnul().

Not that it's a hot path, the bloat-o-meter shows +6 bytes on x86_64, the change seems has no added value, self-rejected.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
build bot
#6 lore raw
In reply to Andy Shevchenko
kernel test robot noticed the following build warnings:

Hi Andy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on akpm-mm/mm-everything]
[also build test WARNING on linus/master v6.5-rc2 next-20230718] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/kasan-Replace-strreplace-with-strchrnul/20230628-233727
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20230628153342.53406-1-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v1 1/1] kasan: Replace strreplace() with strchrnul()
config: x86_64-randconfig-x001-20230718 (https://download.01.org/0day-ci/archive/20230719/[email protected]/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce: (https://download.01.org/0day-ci/archive/20230719/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

>> mm/kasan/report_generic.c:286:3: warning: variable 'p' is uninitialized when used here [-Wuninitialized]
                   p[strchrnul(token, ':') - token] = '\0';
                   ^
   mm/kasan/report_generic.c:267:10: note: initialize the variable 'p' to silence this warning
                   char *p;
                          ^
                           = NULL

1 warning generated.

vim +/p +286 mm/kasan/report_generic.c

242
243 static void print_decoded_frame_descr(const char *frame_descr)
244 {
245 /*
246 * We need to parse the following string:
247 * "n alloc_1 alloc_2 ... alloc_n"
248 * where alloc_i looks like
249 * "offset size len name"
250 * or "offset size len name:line".
251 */
252
253 char token[64];
254 unsigned long num_objects;
255
256 if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
257 &num_objects))
258 return;
259
260 pr_err("\n");
261 pr_err("This frame has %lu %s:\n", num_objects,
262 num_objects == 1 ? "object" : "objects");
263
264 while (num_objects--) {
265 unsigned long offset;
266 unsigned long size;
267 char *p;
268
269 /* access offset */
270 if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
271 &offset))
272 return;
273 /* access size */
274 if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
275 &size))
276 return;
277 /* name length (unused) */
278 if (!tokenize_frame_descr(&frame_descr, NULL, 0, NULL))
279 return;
280 /* object name */
281 if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
282 NULL))
283 return;
284
285 /* Strip line number; without filename it's not very helpful. */

 > 286			p[strchrnul(token, ':') - token] = '\0';

287
288 /* Finally, print object information. */
289 pr_err(" [%lu, %lu) '%s'", offset, offset + size, token);
290 }
291 }
292

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki