Skip to contentSkip to conversation

lib/string_helpers et al.: Change return value of strreplace()

PATCH Applied Andy Shevchenko 2 participants 5 messages Jun 5, 2023

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

v3 cover lib/string_helpers et al.: Change return value of strreplace()

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(-)
Signature · 1 line
2.40.0.1.gaa8946217a0b
#1 lore raw

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

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".

---
 fs/jbd2/journal.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
fs/jbd2/journal.c +2 -4
index 8ae419152ff6..6e17f8f94dfd 100644
@@ -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;
Signature · 1 line
2.40.0.1.gaa8946217a0b
#2 lore raw

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

In reply to 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.

---
 include/linux/string.h |  2 +-
 lib/string_helpers.c   | 12 ++++++++----
 2 files changed, 9 insertions(+), 5 deletions(-)
include/linux/string.h +1 -1
index c062c581a98b..dbfc66400050 100644
@@ -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);
 
lib/string_helpers.c +8 -4
index 230020a2e076..d3b1dd718daf 100644
@@ -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);
 
Signature · 1 line
2.40.0.1.gaa8946217a0b
#3 lore raw

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

In reply to Andy Shevchenko

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

---
 lib/kobject.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
lib/kobject.c +1 -2
index f79a434e1231..16d530f9c174 100644
@@ -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;
Signature · 1 line
2.40.0.1.gaa8946217a0b
Applied
#4 lore raw
In reply to Andy Shevchenko
Quoted 9 lines · 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.

[...]

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
Signature · 1 line
Kees Cook