Messages in this thread | | | Subject | Re: [PATCH] vsprintf: drop comment claiming %n is ignored | From | Joe Perches <> | Date | Fri, 13 Sep 2013 16:23:30 -0700 |
| |
On Fri, 2013-09-13 at 16:03 -0700, Kees Cook wrote: > On Fri, Sep 13, 2013 at 3:27 PM, Joe Perches <joe@perches.com> wrote: > > On Fri, 2013-09-13 at 15:53 -0400, George Spelvin wrote: > >> > Maybe WARN_ONCE so it's easier to emit the format too. > >> > >> Good idea. And, if it's not too much trouble, a comment explaining > >> why it's deliberately omitted so the issue doesn't arise again. > > > > Before any of the %n uses could be removed, I believe seq_printf > > could to be converted to return void and have a another mechanism > > to determine if any error occurred and the length of the output of > > seq_printf. > > > > I've done a preliminary conversion of seq_printf and seq_vprintf > > to return void and added last_ret and last_len to struct seq_file. > > > > If that's applied, it's trivial to convert vsnprintf to skip %n. > > > > Anyone have an opinion of a different conversion mechanism? > > Maybe I missed this somewhere in the thread, but I'm not sure I > understand the move to "void".
Hi Kees.
Al Viro suggested just fixing the misuses. https://lkml.org/lkml/2013/9/11/801
David Laight suggested converting to void. https://lkml.org/lkml/2013/9/12/113
> Here's what I see, please correct me: > > 1- seq_printf currently returns success/failure > 2- some callers of seq_printf (correctly) use the return value as > success/failure indication > 3- some callers of seq_printf (incorrectly) use the return value as a > length indication > 4- both success/failure and length are important outputs from seq_printf > 5- we need a way to access the length written during the call
Right and I agree with all of that.
> 6- want to minimize impact on the code base
Maybe not.
> Due to 1 and 2, it seems like there's no sense in changing the return > value to void. Success/failure is already returned, and there are > users of it. No sense changing them.
Except that future code might expect len instead of bool. I did and would again.
So, I really don't care much and I don't really want to paper over existing misuses, so I choose to fix them all.
> The normal way to handle multiple return values (4 and 5) is to add a > pointer argument. For example: seq_printf(s, &len, fmt, args...) where > len can be NULL. But this runs against 6. > > Due to 6, to solve 4 and 5, usually macro or inline tricks are used, > for example: > __printf(3, 4) int seq_printf_len(struct seq_file *, size_t *len, ...); > #define seq_printf(s, fmt, ...) seq_printf_len(s, NULL, fmt, ##__VA_ARGS__)
I'd rather fix the code that's defective than paper over the defects with macro tricks.
> With this, solving 3 becomes possible (your void patch has already > detected all the users of the return value, so we can sort out which > expect length and which expect success/failure),
I (believe I) did that.
> and lets us actually > remove the %n uses trivially too.
Soon I hope.
Anyone else have an opinion?
| |