[PATCH] Perl: NULL-terminate argument list
mdounin at mdounin.ru
Fri Jun 20 19:41:03 UTC 2014
On Fri, Jun 20, 2014 at 02:46:59AM -0700, Piotr Sikora wrote:
> Hey Maxim,
> > The perlembed manpage is full of examples without terminating
> > NULL, and it's the only documentation available for the
> > perl_parse() function, AFAIK.
> > Could you please elaborate a bit more on the problem the patch
> > tries to fix?
> The problem is that perl_parse() tries to read value at argv[argc]. I
> don't think it uses it, so it doesn't really have to be NULL, but the
> memory must be allocated, otherwise it's reading past the allocation.
> I've started digging into perl's code yesterday, but it's an
> unreadable mess of macros, so I eventually gave up and didn't find
> confirmation for it in the code.
> This issue is quite hard to hit under normal circumstances, because
> nginx uses memory pools, so the 1 byte buffer overrun can only happen
> when argv[argc] == pool->d.last == pool->d.end. Furthermore, you need
> to use malloc that puts guard page right after each allocation,
> otherwise you won't be able to detect it.
> Regarding the perlembed man page, it's indeed lacking any details and
> the examples suggest that the argument list doesn't have to be
> NULL-terminated, however the consistent crashes I was seeing for a few
> configurations (like the one generated by empty_gif.t with an extra
> perl_modules directive passed in via globals) that were fixed with my
> patch suggest otherwise.
> Hope that explains it enough.
Ok, so it looks like a bug either in perl itself, or in the
perlembed manpage. It may make sense to track it further,
probably using valgrind and examples from perlembed.
Most suspicious line I see in perl 5.18 sources is in toke.c:
Copy(PL_origargv+1, newargv+2, PL_origargc+1, char*);
I suspect that "+" in the "PL_origargc+1" is just a typo, it
should be "-". I don't think that suggested patch will help if
it's the reason (or, at least, it won't help in all cases), as it
looks like 2 pointers overrun, not just 1 pointer you are adding.
More information about the nginx-devel