[V2,1/2] string: Add stracpy and stracpy_pad mechanisms
diff mbox series

Message ID ed4611a4a96057bf8076856560bfbf9b5e95d390.1563889130.git.joe@perches.com
State New
Headers show
Series
  • string: Add stracpy and stracpy_pad
Related show

Commit Message

Joe Perches July 23, 2019, 1:51 p.m. UTC
Several uses of strlcpy and strscpy have had defects because the
last argument of each function is misused or typoed.

Add macro mechanisms to avoid this defect.

stracpy (copy a string to a string array) must have a string
array as the first argument (dest) and uses sizeof(dest) as the
count of bytes to copy.

These mechanisms verify that the dest argument is an array of
char or other compatible types like u8 or s8 or equivalent.

A BUILD_BUG is emitted when the type of dest is not compatible.

Signed-off-by: Joe Perches <joe@perches.com>
---

V2: Use __same_type testing char[], signed char[], and unsigned char[]
    Rename to, from, and size, dest, src and count
    Correct return of -E2BIG descriptions

 include/linux/string.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

Comments

Rasmus Villemoes July 23, 2019, 2:37 p.m. UTC | #1
On 23/07/2019 15.51, Joe Perches wrote:
> Several uses of strlcpy and strscpy have had defects because the
> last argument of each function is misused or typoed.
> 
> Add macro mechanisms to avoid this defect.
> 
> stracpy (copy a string to a string array) must have a string
> array as the first argument (dest) and uses sizeof(dest) as the
> count of bytes to copy.
> 
> These mechanisms verify that the dest argument is an array of
> char or other compatible types like u8 or s8 or equivalent.

Sorry, but "compatible types" has a very specific meaning in C, so
please don't use that word. And yes, the kernel disables -Wpointer-sign,
so passing an u8* or s8* when strscpy() expects a char* is silently
accepted, but does such code exist?

> 
> V2: Use __same_type testing char[], signed char[], and unsigned char[]
>     Rename to, from, and size, dest, src and count

count is just as bad as size in terms of "the expression src might
contain that identifier". But there's actually no reason to even declare
a local variable, just use ARRAY_SIZE() directly as the third argument
to strscpy().

Rasmus
Joe Perches July 23, 2019, 3:39 p.m. UTC | #2
On Tue, 2019-07-23 at 16:37 +0200, Rasmus Villemoes wrote:
> On 23/07/2019 15.51, Joe Perches wrote:
> > Several uses of strlcpy and strscpy have had defects because the
> > last argument of each function is misused or typoed.
> > 
> > Add macro mechanisms to avoid this defect.
> > 
> > stracpy (copy a string to a string array) must have a string
> > array as the first argument (dest) and uses sizeof(dest) as the
> > count of bytes to copy.
> > 
> > These mechanisms verify that the dest argument is an array of
> > char or other compatible types like u8 or s8 or equivalent.
> Sorry, but "compatible types" has a very specific meaning in C, so
> please don't use that word.

I think you are being overly pedantic here but
what wording do you actually suggest?

>  And yes, the kernel disables -Wpointer-sign,
> so passing an u8* or s8* when strscpy() expects a char* is silently
> accepted, but does such code exist?

u8 definitely, s8 I'm not sure.

I don't find via grep a use of s8 foo[] = "bar";
or "signed char foo[] = "bar";

I don't think it bad to allow it.

> > V2: Use __same_type testing char[], signed char[], and unsigned char[]
> >     Rename to, from, and size, dest, src and count
> 
> count is just as bad as size in terms of "the expression src might
> contain that identifier". But there's actually no reason to even declare
> a local variable, just use ARRAY_SIZE() directly as the third argument
> to strscpy().

I don't care about that myself.
It's a macro local identifier and shadowing in a macro
is common.  I'm not a big fan of useless underscores.

I think either works.
Rasmus Villemoes July 24, 2019, 6:53 a.m. UTC | #3
On 23/07/2019 17.39, Joe Perches wrote:
> On Tue, 2019-07-23 at 16:37 +0200, Rasmus Villemoes wrote:
>> On 23/07/2019 15.51, Joe Perches wrote:
>>>
>>> These mechanisms verify that the dest argument is an array of
>>> char or other compatible types like u8 or s8 or equivalent.
>> Sorry, but "compatible types" has a very specific meaning in C, so
>> please don't use that word.
> 
> I think you are being overly pedantic here but
> what wording do you actually suggest?

I'd just not support anything other than char[], but if you want,
perhaps say "related types", or some other informal description.

>>  And yes, the kernel disables -Wpointer-sign,
>> so passing an u8* or s8* when strscpy() expects a char* is silently
>> accepted, but does such code exist?
> 
> u8 definitely, s8 I'm not sure.

Example (i.e. of someone passing an u8* as destination to some string
copy/formatting function)? I believe you, I'd just like to see the context.

> I don't find via grep a use of s8 foo[] = "bar";
> or "signed char foo[] = "bar";
> 
> I don't think it bad to allow it.

Your patch.

>> count is just as bad as size in terms of "the expression src might
>> contain that identifier". But there's actually no reason to even declare
>> a local variable, just use ARRAY_SIZE() directly as the third argument
>> to strscpy().
> 
> I don't care about that myself.
> It's a macro local identifier and shadowing in a macro
> is common.  I'm not a big fan of useless underscores.

shadowing is not the problem. The identifier "count" appearing in one of
the "dest" or "src" expressions is. For something that's supposed to
help eliminate bugs, such a hidden footgun seems to be a silly thing to
include. No need for some hideous triple-underscore variable, just make
the whole thing

BUILD_BUG_ON(!__same_type())
strscpy(dst, src, ARRAY_SIZE(dst))

Rasmus
Joe Perches July 24, 2019, 7:10 a.m. UTC | #4
On Wed, 2019-07-24 at 08:53 +0200, Rasmus Villemoes wrote:
> BUILD_BUG_ON(!__same_type())
> strscpy(dst, src, ARRAY_SIZE(dst))

Doing this without the temporary is less legible to read
the compiler error when someone improperly does:

	char *foo;
	char *bar;

	stracpy(foo, bar);

Patch
diff mbox series

diff --git a/include/linux/string.h b/include/linux/string.h
index 4deb11f7976b..7572cd78cf9f 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -35,6 +35,51 @@  ssize_t strscpy(char *, const char *, size_t);
 /* Wraps calls to strscpy()/memset(), no arch specific code required */
 ssize_t strscpy_pad(char *dest, const char *src, size_t count);
 
+/**
+ * stracpy - Copy a C-string into an array of char/u8/s8 or equivalent
+ * @dest: Where to copy the string, must be an array of char and not a pointer
+ * @src: String to copy, may be a pointer or const char array
+ *
+ * Helper for strscpy().
+ * Copies a maximum of sizeof(@dest) bytes of @src with %NUL termination.
+ *
+ * Returns:
+ * * The number of characters copied (not including the trailing %NUL)
+ * * -E2BIG if @dest is a zero size array or @src was truncated.
+ */
+#define stracpy(dest, src)						\
+({									\
+	size_t count = ARRAY_SIZE(dest);				\
+	BUILD_BUG_ON(!(__same_type(dest, char[]) ||			\
+		       __same_type(dest, unsigned char[]) ||		\
+		       __same_type(dest, signed char[])));		\
+									\
+	strscpy(dest, src, count);					\
+})
+
+/**
+ * stracpy_pad - Copy a C-string into an array of char/u8/s8 with %NUL padding
+ * @dest: Where to copy the string, must be an array of char and not a pointer
+ * @src: String to copy, may be a pointer or const char array
+ *
+ * Helper for strscpy_pad().
+ * Copies a maximum of sizeof(@dest) bytes of @src with %NUL termination
+ * and zero-pads the remaining size of @dest
+ *
+ * Returns:
+ * * The number of characters copied (not including the trailing %NUL)
+ * * -E2BIG if @dest is a zero size array or @src was truncated.
+ */
+#define stracpy_pad(dest, src)						\
+({									\
+	size_t count = ARRAY_SIZE(dest);				\
+	BUILD_BUG_ON(!(__same_type(dest, char[]) ||			\
+		       __same_type(dest, unsigned char[]) ||		\
+		       __same_type(dest, signed char[])));		\
+									\
+	strscpy_pad(dest, src, count);					\
+})
+
 #ifndef __HAVE_ARCH_STRCAT
 extern char * strcat(char *, const char *);
 #endif