Re: [PATCH 1/2] remove all uses of printf's %n

[Kees Cook]

On Fri, Sep 20, 2013 at 1:08 AM, Jiri Slaby <jslaby@xxxxxxx> wrote:
> On 09/20/2013 06:09 AM, Tetsuo Handa wrote:
>> --- a/fs/proc/consoles.c
>> +++ b/fs/proc/consoles.c
> ...
>> @@ -47,11 +46,10 @@ static int show_console_dev(struct seq_file *m, void *v)
>>                       con_flags[a].name : ' ';
>>       flags[a] = 0;
>>
>> -     seq_printf(m, "%s%d%n", con->name, con->index, &len);
>> -     len = 21 - len;
>> -     if (len < 1)
>> -             len = 1;
>> -     seq_printf(m, "%*c%c%c%c (%s)", len, ' ', con->read ? 'R' : '-',
>> +     seq_setwidth(m, 21 - 1);
>> +     seq_printf(m, "%s%d", con->name, con->index);
>> +     seq_pad(m, ' ');
>> +     seq_printf(m, "%c%c%c (%s)", con->read ? 'R' : '-',
>>                       con->write ? 'W' : '-', con->unblank ? 'U' : '-',
>>                       flags);
>
> Hello, do you really need seq_setwidth? It makes it really ugly...

There are a few problems that have been discussed on the various
threads. Namely, we want to minimize the changes to the seq_file
structure and to not add additional work to all the seq_file users
that don't care about padding. If the seq_file calls always track how
far they're written across each call, we add unneeded work to all the
users. To avoid this, we must identify to the seq_file subsystem where
we want to start tracking the length written. To allow this to be
spread across multiple calls (something the %n can't do), we must
record seq->count at some point, and then compare against it at the
point where we want to perform padding.

> Or do we need that all? Couldn't we simply have seq_printf_padded? Or
> maybe some % modifier in seq_printf to pad the string?

Adding a _padding version of things means we'd have to add it to all
seq_* function that print things (like printing paths, etc). Using
this method, the output doesn't matter. We declare the starting point,
output whatever we need, then perform padding, and continue writing.

I think the declaration/output/pad method seems the least invasive to
existing users of padding, and the highest level of flexibility going
forward for future users.

>> --- a/net/ipv4/fib_trie.c
>> +++ b/net/ipv4/fib_trie.c
> ...
>> @@ -2548,15 +2549,15 @@ static int fib_route_seq_show(struct seq_file *seq, void *v)
>>                                        (fi->fib_advmss ?
>>                                         fi->fib_advmss + 40 : 0),
>>                                        fi->fib_window,
>> -                                      fi->fib_rtt >> 3, &len);
>> +                                      fi->fib_rtt >> 3);
>>                       else
>>                               seq_printf(seq,
>>                                        "*\t%08X\t%08X\t%04X\t%d\t%u\t"
>> -                                      "%d\t%08X\t%d\t%u\t%u%n",
>> +                                      "%d\t%08X\t%d\t%u\t%u",
>>                                        prefix, 0, flags, 0, 0, 0,
>> -                                      mask, 0, 0, 0, &len);
>> +                                      mask, 0, 0, 0);
>>
>> -                     seq_printf(seq, "%*s\n", 127 - len, "");
>> +                     seq_pad(seq, '\n');
>
> Hmm, seq_pad is unintuitive. I would say it pads the string by '\n'. Of
> course it does not, but...

I don't think this is a very serious problem. Currently, the padding
character is always ' ' for all existing callers, so it only makes
sense to make the trailing character an argument.

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/