Messages in this thread | | | Date | Fri, 7 Nov 2008 08:41:47 +0100 | From | Ingo Molnar <> | Subject | Re: [patch] add /proc/pid/stack to dump task's stack trace |
| |
* Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Thu, Nov 06, 2008 at 04:30:23PM -0800, Ken Chen wrote: > > On Thu, Nov 6, 2008 at 12:35 PM, Ingo Molnar <mingo@elte.hu> wrote: > > >> +static int proc_pid_stack(struct task_struct *task, char *buffer) > > >> +{ > > >> + for (i = 0; i < trace.nr_entries; i++) { > > >> + len += sprintf(buffer + len, "[<%p>] %pS\n", > > >> + (void *)entries[i], (void > > >> *)entries[i]); > > > > > > hm, this looks like a potential buffer overflow - isnt 'buffer' here > > > only valid up to the next PAGE_SIZE boundary? > > So, make trace depth low enough, or even better use seqfiles, if > you're scared by buffer overflows.
it's not about being scared, it's about doing the math: kernel symbols can be up to 128 bytes long, so the per line max becomes 2+2+16+2+1+2+16+128+1 == 170. 4096/170 ~== 24. So without checking we've got guaranteed space for only 24 lines - that's too low.
_In practice_, we'd need a really long trace to trigger it, but i've seen really long traces in the past and this is debug infrastructure, so we cannot take chances here.
> > /* > > + * buffer size used for proc read. See proc_info_read(). > > + * 4K page size but our output routines use some slack for overruns > > + */ > > +#define PROC_BLOCK_SIZE (3*1024)
That sounds like a proper limit - the hard limit for this particular printout function is 4096-170, so we are well within the PROC_BLOCK_SIZE range.
Ingo
| |