This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)
On Mon, 18 Jul 2016, Martin Sebor wrote:
> + /* Number of exponent digits or -1 when unknown. */
> + int expdigs = -1;
> + /* 1 when ARG < 0, 0 when ARG >= 0, -1 when unknown. */
> + int negative = -1;
> + /* Log10 of EXPDIGS. */
> + int logexpdigs = 2;
> +
> + const double log10_2 = .30102999566398119521;
> +
> + if (arg && TREE_CODE (arg) == REAL_CST)
> + {
> + expdigs = real_exponent (TREE_REAL_CST_PTR (arg)) * log10_2;
> + negative = real_isneg (TREE_REAL_CST_PTR (arg));
> + logexpdigs = ilog (expdigs, 10);
This looks wrong for the case of a constant with a negative exponent.
Also, what if the constant has a decimal floating-point type?
> + }
> + else if (REAL_MODE_FORMAT (TYPE_MODE (type))->b == 2)
> + {
> + /* Compute T_MAX_EXP for base 2. */
> + expdigs = REAL_MODE_FORMAT (TYPE_MODE (type))->emax * log10_2;
Shouldn't you also compute logexpdigs here? The comment on logexpdigs
implies it's always meant to have a given relation to expdigs. Or maybe
those variables need to be split into min/max cases, since you're
computing both min/max values below.
> + case 'E':
> + case 'e':
> + /* The minimum output is "[-+]1.234567e+00" for an IEEE double
> + regardless of the value of the actual argument. */
> + res.min = ((0 < negative || spec.get_flag ('+') || spec.get_flag (' '))
> + + 1 /* unit */ + (prec < 0 ? 7 : prec ? prec + 1 : 0)
> + + 2 /* e+ */ + (logexpdigs < 2 ? 2 : logexpdigs));
As I understand your logic, for the case of a constant expdigs and so
logexpdigs may sometimes be 1 bigger than the correct value for that
constant, say for 9.999e+99, so this may not actually be the minimum.
> + /* The maximum output is "-1.234567e+123" for a double and one
> + more byte for a large exponent for a long louble. */
> + res.max = negative < 0 ? res.min + 2 + (spec.get_flag ('L')) : res.min;
No, don't embed such assumptions about particular formats. Compute
correct max/min values based on the exponent range of the format in
question.
> + /* The maximum depends on the magnitude of the value but it's
> + at most 316 bytes for double and 4940 for long double, plus
> + precision if non-negative, or 6. */
I don't think comments should embed such assumptions, either.
> + case 'G':
> + case 'g':
> + /* Treat this the same as '%F' for now even though that's
> + inaccurate. */
> + res.min = 2 + (prec < 0 ? 6 : prec);
> + res.max = ((spec.get_flag ('L') ? 4934 : 310)
> + + (prec < 0 ? 6 : prec ? prec + 1 : 0));
Again, avoid embedding properties of given formats.
(I realise tree-call-cdce.c embeds some such assumptions, but that's bad
practice that should not be repeated.)
--
Joseph S. Myers
joseph@codesourcery.com