Messages in this thread Patch in this message | | | Subject | Re: [PATCH] vsprintf: drop comment claiming %n is ignored | From | Tetsuo Handa <> | Date | Sat, 14 Sep 2013 11:49:51 +0900 |
| |
Kees Cook wrote: > 3- some callers of seq_printf (incorrectly) use the return value as a > length indication
Are there really?
Is somebody using the return value from seq_printf() like
pos = snprintf(buf, sizeof(buf) - 1, "%s", foo); snprintf(buf + pos, sizeof(buf) - 1 - pos, "%s", bar);
? Since the caller cannot pass the return value from seq_printf() like
pos = seq_printf(m, "%s", foo); seq_printf(m + pos, "%s", bar);
, I wonder who would interpret the return value as a length indication.
Even bad code which has never tested failure case, the authors should already know that "seq_printf() returns 0 on success case".
I think that
pos += seq_printf(m, "%s", foo); pos += seq_printf(m, "%s", bar);
is used as the equivalent to
if (seq_printf(m, "%s", foo)) pos = -1; if (seq_printf(m, "%s", bar)) pos = -1;
.
Joe Perches wrote: > @@ -174,8 +171,8 @@ static int dbg_show_state(struct seq_file *s, void *p) > int pos = 0; > > /* basic device status */ > - pos += seq_printf(s, "DMA engine status\n"); > - pos += seq_printf(s, "\tChannel number: %d\n", num_dma_channels); > + seq_puts(s, "DMA engine status\n"); > + seq_printf(s, "\tChannel number: %d\n", num_dma_channels); > > return pos; > }
As I described above, I think this change breaks the functionality. We need to change like
- pos += seq_printf(s, "DMA engine status\n"); - pos += seq_printf(s, "\tChannel number: %d\n", num_dma_channels); + pos |= seq_puts(s, "DMA engine status\n"); + pos |= seq_printf(s, "\tChannel number: %d\n", num_dma_channels);
or
- pos += seq_printf(s, "DMA engine status\n"); - pos += seq_printf(s, "\tChannel number: %d\n", num_dma_channels); + seq_puts(s, "DMA engine status\n"); + seq_printf(s, "\tChannel number: %d\n", num_dma_channels); - return pos; + return seq_overflow(s) : -1 : 0;
for keeping the functionality.
| |