[PATCH] Perl: NULL-terminate argument list

[Maxim Dounin]

Hello!

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.

-- 
Maxim Dounin
http://nginx.org/