Обсуждение: Seg-fault in format(text)

Testing 9.1beta:

select format('Hello %s, %2147483648$s', 'World');
server closed the connection unexpectedly
    This probably means the server terminated abnormally
    before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

The problem is that the test for overflow of the arg position doesn't
catch all cases. The simplest solution is to just tweak the comparison
at varlena.c:3840 (patch attached) although maybe there are neater
ways...

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> Testing 9.1beta:
> select format('Hello %s, %2147483648$s', 'World');
> server closed the connection unexpectedly

Yeah, same here.

>               do
>               {
>                   /* Treat overflowing arg position as unterminated. */
> !                 if (arg > INT_MAX / 10)
>                       break;
>                   arg = arg * 10 + (*cp - '0');
>                   ++cp;
> --- 3837,3843 ----
>               do
>               {
>                   /* Treat overflowing arg position as unterminated. */
> !                 if (arg >= INT_MAX / 10)
>                       break;
>                   arg = arg * 10 + (*cp - '0');
>                   ++cp;

Not sure I trust this fix to catch all cases --- seems like the addition
could still overflow.  It'd probably be better if we made this code look
like the overflow test in scanint8:

        int64        newtmp = tmp * 10 + (*ptr++ - '0');

        if ((newtmp / 10) != tmp)        /* overflow? */


            regards, tom lane

On 23.05.2011 17:33, Tom Lane wrote:
> Not sure I trust this fix to catch all cases --- seems like the addition
> could still overflow.  It'd probably be better if we made this code look
> like the overflow test in scanint8:
>
>         int64        newtmp = tmp * 10 + (*ptr++ - '0');
>
>         if ((newtmp / 10) != tmp)        /* overflow? */

It might be good to replace the loop altogether with strtoul() here, for
readability purposes if nothing else.

I also note that there's no overflow check in plain "%s" case which does
"++arg". It seems impossible to cause that overflow, because a function
can't have more than 100 arguments, but it doesn't make me feel very
comfortable.

It's actually possible to pass more than 100 arguments using the
VARIADIC keyword, along the lines of:

CREATE FUNCTION my_concat(variadic text[]) RETURNS text LANGUAGE SQL AS
$$ SELECT array_to_string($1,''); $$;

SELECT my_concat(VARIADIC (SELECT array_agg(a::text) FROM
generate_series(1,1000) a));

However, built-in concat and text_format functions don't seem to behave
that way:

postgres=# SELECT my_concat(VARIADIC (SELECT array_agg(a::text) FROM
generate_series(1,10) a));
   my_concat
-------------
  12345678910
(1 row)

postgres=# SELECT concat(VARIADIC (SELECT array_agg(a::text) FROM
generate_series(1,10) a));
          concat
------------------------
  {1,2,3,4,5,6,7,8,9,10}
(1 row)

I'm not sure what's going on here, something funny with the fact that
those take an 'any' argument, I guess.

It's a stretch that we'd ever support two billion arguments in any form,
but an explicit check for "arg < 0" would make me feel better.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> It's a stretch that we'd ever support two billion arguments in any form,
> but an explicit check for "arg < 0" would make me feel better.

No objection here, especially since I've just determined that bug #6035
is also some kind of unhandled-overflow problem...

Do you want to take care of this one?

            regards, tom lane

On 23.05.2011 18:36, Tom Lane wrote:
> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>  writes:
>> It's a stretch that we'd ever support two billion arguments in any form,
>> but an explicit check for "arg<  0" would make me feel better.
>
> No objection here, especially since I've just determined that bug #6035
> is also some kind of unhandled-overflow problem...
>
> Do you want to take care of this one?

Ok.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

On 23.05.2011 18:36, Tom Lane wrote:
> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>  writes:
>> It's a stretch that we'd ever support two billion arguments in any form,
>> but an explicit check for "arg<  0" would make me feel better.
>
> No objection here, especially since I've just determined that bug #6035
> is also some kind of unhandled-overflow problem...

Committed, using your suggested fix. I realized that switching to
strtoul() wouldn't work without some further changes, because the format
string is not null-terminated.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com