Horton

Thread

Commits

  1. Ensure that snprintf.c's fmtint() doesn't overflow when printing INT64_MIN.

  1. overflow in snprintf() when printing INT64_MIN

    Andres Freund <andres@anarazel.de> — 2018-09-28T00:11:21Z

    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

  2. Re: overflow in snprintf() when printing INT64_MIN

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-28T00:18:12Z

    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

  3. Re: overflow in snprintf() when printing INT64_MIN

    Andres Freund <andres@anarazel.de> — 2018-09-28T00:34:54Z

    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

  4. Re: overflow in snprintf() when printing INT64_MIN

    Andres Freund <andres@anarazel.de> — 2018-09-28T00:44:59Z

    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