Обсуждение: 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