Обсуждение: overflow in snprintf() when printing INT64_MIN

Hi,

I just noticed, while reviewing a patch that corrects overflow handing
in snprintf, that we don't correctly handle INT64_MIN in snprintf.c:

static void
fmtint(int64 value, char type, int forcesign, int leftjust,
           int minlen, int zpad, int precision, int pointflag,
           PrintfTarget *target)
{
...
        /* Handle +/- */
        if (dosign && adjust_sign((value < 0), forcesign, &signvalue))
                value = -value;

If value already is INT64_MIN this can't work.  It just happens to fail
to fail, because the later cast with (uint64) value "hides" the damage.

I suspect the best way to fix this, would be to instead do:

    /* Handle +/- */
    if (dosign && adjust_sign((value < 0), forcesign, &signvalue);
        uvalue = -(uint64) value;
    else
        uvalue = (uint64) value;

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> I just noticed, while reviewing a patch that corrects overflow handing
> in snprintf, that we don't correctly handle INT64_MIN in snprintf.c:

Well, you still get the right answer, even if the "-value" is
nominally undefined.

> I suspect the best way to fix this, would be to instead do:

>     /* Handle +/- */
>     if (dosign && adjust_sign((value < 0), forcesign, &signvalue);
>         uvalue = -(uint64) value;
>     else
>         uvalue = (uint64) value;

Hm, what does -x mean for an unsigned value?  I'm not really
convinced this is conceptually better.

            regards, tom lane

Hi,

On 2018-09-27 20:18:12 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I just noticed, while reviewing a patch that corrects overflow handing
> > in snprintf, that we don't correctly handle INT64_MIN in snprintf.c:
> 
> Well, you still get the right answer, even if the "-value" is
> nominally undefined.

Right.


> > I suspect the best way to fix this, would be to instead do:
> 
> >     /* Handle +/- */
> >     if (dosign && adjust_sign((value < 0), forcesign, &signvalue);
> >         uvalue = -(uint64) value;
> >     else
> >         uvalue = (uint64) value;
> 
> Hm, what does -x mean for an unsigned value?  I'm not really
> convinced this is conceptually better.

6.2.5 (9): "... A computation involving unsigned operands can never
overflow, because a result that cannot be represented by the resulting
unsigned integer type is reduced modulo the number that is one greater
than the largest value that can be represented by the resulting type."

(unsigned)((int)-1) == 4294967295
-(unsigned)4294967295 == 1

I think that's well defined.

Greetings,

Andres Freund

Hi,

On 2018-09-27 17:34:54 -0700, Andres Freund wrote:
> On 2018-09-27 20:18:12 -0400, Tom Lane wrote:
> > >     /* Handle +/- */
> > >     if (dosign && adjust_sign((value < 0), forcesign, &signvalue);
> > >         uvalue = -(uint64) value;
> > >     else
> > >         uvalue = (uint64) value;
> > 
> > Hm, what does -x mean for an unsigned value?  I'm not really
> > convinced this is conceptually better.
> 
> 6.2.5 (9): "... A computation involving unsigned operands can never
> overflow, because a result that cannot be represented by the resulting
> unsigned integer type is reduced modulo the number that is one greater
> than the largest value that can be represented by the resulting type."
> 
> (unsigned)((int)-1) == 4294967295
> -(unsigned)4294967295 == 1
> 
> I think that's well defined.

I guess some might consider
  uvalue = (uint64) 0 - (uint64) value
to be easier to reason about?

Greetings,

Andres Freund