diff mbox series

[1/4] string: try to find const-laundering bugs

Message ID 20180822110009.17583-1-linux@rasmusvillemoes.dk
State New, archived
Headers show
Series [1/4] string: try to find const-laundering bugs | expand

Commit Message

Rasmus Villemoes Aug. 22, 2018, 11 a.m. UTC
This wraps strchr and friends in macros that ensure the return value has
type const char* if the passed-in string (which the return value points
into) also has type const char*. The (s)+0 thing is to force a const
char[] (e.g. a string literal) to decay to a const char* for the
__same_type comparison.

Not sure it's worth the churn necessary to eliminate the resulting
warnings, but maybe it'll find some actual bugs. It certainly spews
out lots of warnings all over the tree, but instead of trying to fix
them up before actually getting this patch in, I've made the use of
these macros opt-in so anybody can do a build with
KCFLAGS=-D__CONST_LAUNDER (or add a 'ccflags-y += ...' to some
sub-Makefile) and take a look at their subsystem of interest.

Note that the use of the identifier strchr in the macro strchr is not a
problem; the preprocessor rules say that the expanded strchr should be
left alone. But we do have to prevent macro expansion when actually
defining the strchr function, hence the lib/string.c part of this. For
the same reason, I'm not defining strchr when __HAVE_ARCH_STRCHR, since
then I'd have to go into all those arches and protect their definitions
similarly.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
Patches 2,3,4 can be applied independently of each other and this one,
and just serve as examples of the kind of churn enabling these
unconditionally would give.

While playing with this, I haven't found any obvious bugs, but one
example where it's not immediately obvious whether there is a problem
is

	comma = strchr(dev_name, ',');
	if (comma != NULL && comma < end)
		*comma = 0;

in fs/nfs/super.c. dev_name is const char*, but it is modified through
comma. I haven't been able to track back through all the VFS callbacks
whether that's perfectly fine. At least a comment would be nice.

 include/linux/string.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 lib/string.c           | 12 ++++++------
 2 files changed, 53 insertions(+), 6 deletions(-)

Comments

Joe Perches Aug. 22, 2018, 11:07 a.m. UTC | #1
On Wed, 2018-08-22 at 13:00 +0200, Rasmus Villemoes wrote:
> This wraps strchr and friends in macros that ensure the return value has
> type const char* if the passed-in string (which the return value points
> into) also has type const char*. The (s)+0 thing is to force a const
> char[] (e.g. a string literal) to decay to a const char* for the
> __same_type comparison.
[]
> diff --git a/include/linux/string.h b/include/linux/string.h
[]
> +#define strchr(s, c) (						  \
> +	__builtin_choose_expr(__same_type((s) + 0, const char *), \
> +			      (const char *)strchr(s, c),	  \
> +			      strchr(s, c)))
> +#endif
[]
> diff --git a/lib/string.c b/lib/string.c
[]
> @@ -367,7 +367,7 @@ EXPORT_SYMBOL(strncmp);
>   * @s: The string to be searched
>   * @c: The character to search for
>   */
> -char *strchr(const char *s, int c)
> +char *(strchr)(const char *s, int c)

I've tried to use this macro/function wrapping
a few times before, but it seems that it's fairly
unusual in the kernel.  I believe there may not
be any current uses of that style.

A comment explaining the form might be useful.
Rasmus Villemoes Aug. 22, 2018, 11:40 a.m. UTC | #2
On 2018-08-22 13:07, Joe Perches wrote:
> On Wed, 2018-08-22 at 13:00 +0200, Rasmus Villemoes wrote:
>> This wraps strchr and friends in macros that ensure the return value has
>> type const char* if the passed-in string (which the return value points
>> into) also has type const char*. The (s)+0 thing is to force a const
>> char[] (e.g. a string literal) to decay to a const char* for the
>> __same_type comparison.
> []
>> diff --git a/include/linux/string.h b/include/linux/string.h
> []
>> +#define strchr(s, c) (						  \
>> +	__builtin_choose_expr(__same_type((s) + 0, const char *), \
>> +			      (const char *)strchr(s, c),	  \
>> +			      strchr(s, c)))
>> +#endif
> []
>> diff --git a/lib/string.c b/lib/string.c
> []
>> @@ -367,7 +367,7 @@ EXPORT_SYMBOL(strncmp);
>>   * @s: The string to be searched
>>   * @c: The character to search for
>>   */
>> -char *strchr(const char *s, int c)
>> +char *(strchr)(const char *s, int c)
> 
> I've tried to use this macro/function wrapping
> a few times before, but it seems that it's fairly
> unusual in the kernel.  I believe there may not
> be any current uses of that style.
> 
> A comment explaining the form might be useful.
> 

True. I dislike the more conventional #undef strchr approach, because
that would mean any other function in string.c that calls strchr and
happens to be defined after strchr() would not be 'instrumented'. It's
more a principle than a practical thing because, well, none of the
instrumented functions happen to be used by other functions in string.c.

Rasmus
diff mbox series

Patch

diff --git a/include/linux/string.h b/include/linux/string.h
index 4a5a0eb7df51..06d260cdc4b7 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -449,4 +449,51 @@  static inline void memcpy_and_pad(void *dest, size_t dest_len,
 		memcpy(dest, src, dest_len);
 }
 
+#ifdef __CONST_LAUNDER
+
+#ifndef __HAVE_ARCH_STRCHR
+#define strchr(s, c) (						  \
+	__builtin_choose_expr(__same_type((s) + 0, const char *), \
+			      (const char *)strchr(s, c),	  \
+			      strchr(s, c)))
+#endif
+
+#ifndef __HAVE_ARCH_STRCHRNUL
+#define strchrnul(s, c) (					  \
+	__builtin_choose_expr(__same_type((s) + 0, const char *), \
+			      (const char *)strchrnul(s, c),	  \
+			      strchrnul(s, c)))
+#endif
+
+#ifndef __HAVE_ARCH_STRNCHR
+#define strnchr(s, n, c) (					  \
+	__builtin_choose_expr(__same_type((s) + 0, const char *), \
+			      (const char *)strnchr(s, n, c),	  \
+			      strnchr(s, n, c)))
+#endif
+
+#ifndef __HAVE_ARCH_STRRCHR
+#define strrchr(s, c) (						  \
+	__builtin_choose_expr(__same_type((s) + 0, const char *), \
+			      (const char *)strrchr(s, c),	  \
+			      strrchr(s, c)))
+#endif
+
+#ifndef __HAVE_ARCH_STRSTR
+#define strstr(s, t) (						  \
+	__builtin_choose_expr(__same_type((s) + 0, const char *), \
+			      (const char *)strstr(s, t),	  \
+			      strstr(s, t)))
+#endif
+
+#ifndef __HAVE_ARCH_STRNSTR
+#define strnstr(s, t, n) (					  \
+	__builtin_choose_expr(__same_type((s) + 0, const char *), \
+			      (const char *)strnstr(s, t, n),	  \
+			      strnstr(s, t, n)))
+#endif
+
+#endif /* __CONST_LAUNDER */
+
+
 #endif /* _LINUX_STRING_H_ */
diff --git a/lib/string.c b/lib/string.c
index 2c0900a5d51a..89871c6d57cf 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -367,7 +367,7 @@  EXPORT_SYMBOL(strncmp);
  * @s: The string to be searched
  * @c: The character to search for
  */
-char *strchr(const char *s, int c)
+char *(strchr)(const char *s, int c)
 {
 	for (; *s != (char)c; ++s)
 		if (*s == '\0')
@@ -386,7 +386,7 @@  EXPORT_SYMBOL(strchr);
  * Returns pointer to first occurrence of 'c' in s. If c is not found, then
  * return a pointer to the null byte at the end of s.
  */
-char *strchrnul(const char *s, int c)
+char *(strchrnul)(const char *s, int c)
 {
 	while (*s && *s != (char)c)
 		s++;
@@ -401,7 +401,7 @@  EXPORT_SYMBOL(strchrnul);
  * @s: The string to be searched
  * @c: The character to search for
  */
-char *strrchr(const char *s, int c)
+char *(strrchr)(const char *s, int c)
 {
 	const char *last = NULL;
 	do {
@@ -420,7 +420,7 @@  EXPORT_SYMBOL(strrchr);
  * @count: The number of characters to be searched
  * @c: The character to search for
  */
-char *strnchr(const char *s, size_t count, int c)
+char *(strnchr)(const char *s, size_t count, int c)
 {
 	for (; count-- && *s != '\0'; ++s)
 		if (*s == (char)c)
@@ -896,7 +896,7 @@  EXPORT_SYMBOL(memscan);
  * @s1: The string to be searched
  * @s2: The string to search for
  */
-char *strstr(const char *s1, const char *s2)
+char *(strstr)(const char *s1, const char *s2)
 {
 	size_t l1, l2;
 
@@ -922,7 +922,7 @@  EXPORT_SYMBOL(strstr);
  * @s2: The string to search for
  * @len: the maximum number of characters to search
  */
-char *strnstr(const char *s1, const char *s2, size_t len)
+char *(strnstr)(const char *s1, const char *s2, size_t len)
 {
 	size_t l2;