Messages in this thread Patch in this message | | | Date | Tue, 28 Aug 2018 22:35:02 +0300 | From | Alexey Dobriyan <> | Subject | Re: [PATCH 11/13] proc: readdir /proc/*/task |
| |
On Tue, Aug 28, 2018 at 01:04:40PM +0000, Ahmed S. Darwish wrote: > On Tue, Aug 28, 2018 at 12:36:22PM +0000, Ahmed S. Darwish wrote: > > On Tue, Aug 28, 2018 at 02:15:01AM +0300, Alexey Dobriyan wrote: > > > --- > > > fs/proc/base.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > Missing description and S-o-b. Further comments below.. > > > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > > index 33f444721965..668e465c86b3 100644 > > > --- a/fs/proc/base.c > > > +++ b/fs/proc/base.c > > > @@ -3549,11 +3549,11 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) > > > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); > > > task; > > > task = next_tid(task), ctx->pos++) { > > > - char name[10 + 1]; > > > - unsigned int len; > > > + char name[10], *p = name + sizeof(name); > > > + > > > > Multiple issues: > > > > - len should be 11, as was in the original code > > (0xffffffff = 4294967295, 10 letters) > > > > - while we're at it, let's use a constant for the '11' instead of > > mysterious magic numbers > > > > - 'p' is clearly overflowing the stack here > > > > See below: > > > > tid = task_pid_nr_ns(task, ns); > > > - len = snprintf(name, sizeof(name), "%u", tid); > > > - if (!proc_fill_cache(file, ctx, name, len, > > > + p = _print_integer_u32(p, tid); > > > + if (!proc_fill_cache(file, ctx, p, name + sizeof(name) - p, > > > > You're replacing snprintf() code __that did proper len checking__ > > with code that does not. That's not good. > > > > I can't see how the fourth proc_fill_cache() parameter, ``name + > > sizeof(name)'' safely ever replace the original 'len' parameter. > > It's a pointer value .. (!) > > > > Ok, there's a "- p" in the end, so the length looks to be Ok. > > Nonetheless, the whole patch series is introducing funny code > like: > > +/* > + * Print an integer in decimal. > + * "p" initially points PAST THE END OF THE BUFFER! > + * > + * DO NOT USE THESE FUNCTIONS! > + * > + * Do not copy these functions. > + * Do not document these functions. > + * Do not move these functions to lib/ or elsewhere. > + * Do not export these functions to modules. > + * Do not tell anyone about these functions. > + */ > +noinline > +char *_print_integer_u32(char *p, u32 x) > +{ > + do { > + *--p = '0' + (x % 10); > + x /= 10; > + } while (x != 0); > + return p; > +} > > And thus the code using these functions is throwing invalid > past-the-stack pointers and strings with no NULL terminators > like there's no tomorrow... > > IMHO It's an accident waiting to happen to sprinkle pointers > like that everywhere.
It is not if people will be prohibited from moving this code to lib/ and "improving" it by adding more parameters.
> Are we really in a super hot path to justify all that?
/proc is very slow, try profiling just about anything involving /proc.
| |