Re: [RFC 0/7] eliminate snprintf with overlapping src and dst

[Rasmus Villemoes]

On Wed, Mar 09 2016, Andrew Morton <a...@linux-foundation.org> wrote:

> On Tue,  8 Mar 2016 21:40:47 +0100 Rasmus Villemoes 
> <li...@rasmusvillemoes.dk> wrote:
>
>> Doing snprintf(buf, len, "%s...", buf, ...) for appending to a buffer
>> currently works, but it is somewhat fragile, and any other overlap
>> between source and destination buffers would be a definite bug. This
>> is an attempt at eliminating the relatively few occurences of this
>> pattern in the kernel.
>
> I dunno,
>
>       snprintf(analog->name, sizeof(analog->name), "Analog %d-axis %d-button",
>
> is pretty damn convenient.  Can we instead state that "sprintf shall
> support this"?  Maybe add a little __init testcase to vsprintf.c to
> check that it continues to work OK.

As Andy points out (thanks!), we actually already have an interface for
simple managing of a user-supplied buffer, seq_buf, which is at least as
convenient, and also avoids the manual bookkeeping that I changed it
into.

OK, one problem is that seq_buf_puts doesn't actually produce a
'\0'-terminated string, but since there's no in-tree users of
seq_buf_puts currently, I think we can easily fix that. Then the rule
would be that as long as one only uses the "string" functions
seq_buf_puts and seq_buf_printf one gets a '\0'-terminated string, while
any use of seq_buf_putc, seq_buf_putmem etc. will void that property.

For the joystick case, this is roughly what it would look like. I think
it's nice to avoid passing the analog->name, sizeof(analog->name) pair
every time.


diff --git a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c
index 6f8b084e13d0..e69ff4d3e31a 100644
--- a/drivers/input/joystick/analog.c
+++ b/drivers/input/joystick/analog.c
@@ -37,6 +37,7 @@
 #include <linux/jiffies.h>
 #include <linux/timex.h>
 #include <linux/timekeeping.h>
+#include <linux/seq_buf.h>
 
 #define DRIVER_DESC    "Analog joystick and gamepad driver"
 
@@ -435,23 +436,24 @@ static void analog_calibrate_timer(struct analog_port 
*port)
 
 static void analog_name(struct analog *analog)
 {
-       snprintf(analog->name, sizeof(analog->name), "Analog %d-axis %d-button",
-                hweight8(analog->mask & ANALOG_AXES_STD),
-                hweight8(analog->mask & ANALOG_BTNS_STD) + !!(analog->mask & 
ANALOG_BTNS_CHF) * 2 +
-                hweight16(analog->mask & ANALOG_BTNS_GAMEPAD) + 
!!(analog->mask & ANALOG_HBTN_CHF) * 4);
+       struct seq_buf sb;
+
+       seq_buf_init(&sb, analog->name, sizeof(analog->name));
+
+       seq_buf_printf(&sb, "Analog %d-axis %d-button",
+               hweight8(analog->mask & ANALOG_AXES_STD),
+               hweight8(analog->mask & ANALOG_BTNS_STD) + !!(analog->mask & 
ANALOG_BTNS_CHF) * 2 +
+               hweight16(analog->mask & ANALOG_BTNS_GAMEPAD) + !!(analog->mask 
& ANALOG_HBTN_CHF) * 4);
 
        if (analog->mask & ANALOG_HATS_ALL)
-               snprintf(analog->name, sizeof(analog->name), "%s %d-hat",
-                        analog->name, hweight16(analog->mask & 
ANALOG_HATS_ALL));
+               seq_buf_printf(&sb, " %d-hat", hweight16(analog->mask & 
ANALOG_HATS_ALL));
 
        if (analog->mask & ANALOG_HAT_FCS)
-               strlcat(analog->name, " FCS", sizeof(analog->name));
+               seq_buf_puts(&sb, " FCS");
        if (analog->mask & ANALOG_ANY_CHF)
-               strlcat(analog->name, (analog->mask & ANALOG_SAITEK) ? " 
Saitek" : " CHF",
-                       sizeof(analog->name));
+               seq_buf_puts(&sb, (analog->mask & ANALOG_SAITEK) ? " Saitek" : 
" CHF");
 
-       strlcat(analog->name, (analog->mask & ANALOG_GAMEPAD) ? " gamepad": " 
joystick",
-               sizeof(analog->name));
+       seq_buf_puts(&sb, (analog->mask & ANALOG_GAMEPAD) ? " gamepad": " 
joystick");
 }
 
 /*