[PATCH] perl module: fix SSI parameter termination bug

[Alexandr Gomoliako]

> The null-terminated string in handler is required for
>
>     sv = newSVpvn((char *) handler->data, handler->len);
>
> below though, and your patch will break it.

This is incorrect, newSVpvn creates a new SV and copies header->len
bytes from header->data into it. It never does strlen() on this data
and always allocates extra byte itself. So, something like this is
fine:

    newSVpvn("\0\0\0", 3);

> Hmm, I was going by this info in the perlapi documentation, which made
> me think that newSVpvn didn't need it:

> "Notice that you can choose to specify the length of the string to be
> assigned by using sv_setpvn , newSVpvn , or newSVpv , or you may allow
> Perl to calculate the length by using sv_setpv or by specifying 0 as
> the second argument to newSVpv . Be warned, though, that Perl will
> determine the string's length by using strlen , which depends on the
> string terminating with a NUL character."

> However I see below that it says:
> "All SVs that contain strings should be terminated with a NUL
> character. If it is not NUL-terminated there is a risk of core dumps
> and corruptions from code which passes the string to C functions or
> system calls which expect a NUL-terminated string. Perl's own
> functions typically add a trailing NUL for this reason."

Notice how it says, that "Perl's own functions typically add a
trailing NUL for this reason.", this includes newSVpvn.

>> Correct solution would be to reserve space for NUL character in
>> ssi module instead.  Quick and dirty patch which should work:

>> --- a/src/http/modules/ngx_http_ssi_filter_module.c
>> +++ b/src/http/modules/ngx_http_ssi_filter_module.c
>> @@ -1204,7 +1204,7 @@ ngx_http_ssi_parse(ngx_http_request_t *r
>>
>>                 if (ctx->value_buf == NULL) {
>>                     ctx->param->value.data = ngx_pnalloc(r->pool,
>> -                                                         ctx->value_len);
>> +                                                         ctx->value_len +
>> 1);
>>                     if (ctx->param->value.data == NULL) {
>>                         return NGX_ERROR;
>>                     }
>>
>
> I wasn't sure if changing the SSI allocation code was appropriate in
> this case, but since newSVpvn really does need the NULL termination,
> this is indeed a better idea.

No, it's not. The only solution is to fix it in ssi module, as
proposed by Maxim.