[PATCH v3 0/3] lib/string_helpers et al.: Change return value of strreplace()

[PATCH v3 0/3] lib/string_helpers et al.: Change return value of strreplace()

[Andy Shevchenko]

It's more convenient to have strreplace() to return the pointer to
 the string itself. This will help users to make their code better.

The patch 1 kills the only user of the returned value of strreplace(),
Patch 2 converts the return value of strreplace(). And patch 3 shows
how it may be useful. That said, the series can be routed via fs tree,
with or without the last patch.

In v3:
- rebased on top of latest Linux Next

In v2:
- removed not anymore used variable (LKP)
- added tag (Jan)
- fixed wording (Kees)
- actually return the pointer to the string itself

Andy Shevchenko (3):
  jbd2: Avoid printing outside the boundary of the buffer
  lib/string_helpers: Change returned value of the strreplace()
  kobject: Use return value of strreplace()

 fs/jbd2/journal.c      |  6 ++----
 include/linux/string.h |  2 +-
 lib/kobject.c          |  3 +--
 lib/string_helpers.c   | 12 ++++++++----
 4 files changed, 12 insertions(+), 11 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

[PATCH v3 1/3] jbd2: Avoid printing outside the boundary of the buffer

[Andy Shevchenko]

Theoretically possible that "%pg" will take all room for the j_devname
and hence the "-%lu" will go outside the boundary due to unconditional
sprintf() in use. To make this code more robust, replace two sequential
s*printf():s by a single call and then replace forbidden character.
It's possible to do this way, because '/' won't ever be in the result
of "-%lu".

Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 fs/jbd2/journal.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 8ae419152ff6..6e17f8f94dfd 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1491,7 +1491,6 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
 {
 	journal_t *journal;
 	sector_t blocknr;
-	char *p;
 	int err = 0;
 
 	blocknr = 0;
@@ -1515,9 +1514,8 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
 
 	journal->j_inode = inode;
 	snprintf(journal->j_devname, sizeof(journal->j_devname),
-		 "%pg", journal->j_dev);
-	p = strreplace(journal->j_devname, '/', '!');
-	sprintf(p, "-%lu", journal->j_inode->i_ino);
+		 "%pg-%lu", journal->j_dev, journal->j_inode->i_ino);
+	strreplace(journal->j_devname, '/', '!');
 	jbd2_stats_proc_init(journal);
 
 	return journal;
-- 
2.40.0.1.gaa8946217a0b

[PATCH v3 2/3] lib/string_helpers: Change returned value of the strreplace()

[Andy Shevchenko]

It's more useful to return the pointer to the string itself
with strreplace(), so it may be used like

	attr->name = strreplace(name, '/', '_');

While at it, amend the kernel documentation.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/string.h |  2 +-
 lib/string_helpers.c   | 12 ++++++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index c062c581a98b..dbfc66400050 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -169,7 +169,7 @@ static inline void memcpy_flushcache(void *dst, const void *src, size_t cnt)
 #endif
 
 void *memchr_inv(const void *s, int c, size_t n);
-char *strreplace(char *s, char old, char new);
+char *strreplace(char *str, char old, char new);
 
 extern void kfree_const(const void *x);
 
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 230020a2e076..d3b1dd718daf 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -979,18 +979,22 @@ EXPORT_SYMBOL(__sysfs_match_string);
 
 /**
  * strreplace - Replace all occurrences of character in string.
- * @s: The string to operate on.
+ * @str: The string to operate on.
  * @old: The character being replaced.
  * @new: The character @old is replaced with.
  *
- * Returns pointer to the nul byte at the end of @s.
+ * Replaces the each @old character with a @new one in the given string @str.
+ *
+ * Return: pointer to the string @str itself.
  */
-char *strreplace(char *s, char old, char new)
+char *strreplace(char *str, char old, char new)
 {
+	char *s = str;
+
 	for (; *s; ++s)
 		if (*s == old)
 			*s = new;
-	return s;
+	return str;
 }
 EXPORT_SYMBOL(strreplace);
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v3 3/3] kobject: Use return value of strreplace()

[Andy Shevchenko]

Since strreplace() returns the pointer to the string itself,
we may use it directly in the code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/kobject.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index f79a434e1231..16d530f9c174 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -281,8 +281,7 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 		kfree_const(s);
 		if (!t)
 			return -ENOMEM;
-		strreplace(t, '/', '!');
-		s = t;
+		s = strreplace(t, '/', '!');
 	}
 	kfree_const(kobj->name);
 	kobj->name = s;
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v3 0/3] lib/string_helpers et al.: Change return value of strreplace()

[Kees Cook]

On Mon, 5 Jun 2023 20:05:50 +0300, Andy Shevchenko wrote:
> It's more convenient to have strreplace() to return the pointer to
>  the string itself. This will help users to make their code better.
> 
> The patch 1 kills the only user of the returned value of strreplace(),
> Patch 2 converts the return value of strreplace(). And patch 3 shows
> how it may be useful. That said, the series can be routed via fs tree,
> with or without the last patch.
> 
> [...]

Applied to for-next/hardening, thanks!

[1/3] jbd2: Avoid printing outside the boundary of the buffer
      https://git.kernel.org/kees/c/7afb6d8fa81f
[2/3] lib/string_helpers: Change returned value of the strreplace()
      https://git.kernel.org/kees/c/d01a77afd6be
[3/3] kobject: Use return value of strreplace()
      https://git.kernel.org/kees/c/b2f10148ec1e

-- 
Kees Cook