- improve sprintf buffer overflow detection (middle-end/49905)

On 08/23/2016 05:41 PM, Manuel López-Ibáñez wrote:
> On 23 August 2016 at 22:56, Martin Sebor <msebor@gmail.com> wrote:
>> Attached is the latest patch.
>
> Some suggestions:

Thanks for the detailed comments!

>
> --Wno-format-contains-nul -Wno-format-extra-args -Wformat-nonliteral @gol
> +-Wno-format-contains-nul -Wno-format-extra-args -Wformat-length=1 @gol
>
> Most options that take levels are documented as:
>
> -Wshift-overflow -Wshift-overflow=@var{n}
> -Wstrict-overflow -Wstrict-overflow=@var{n}

I haven't found any uses of the @var{} tag for numeric arguments
to options (e.g., -Wformat=2), only for symbolic arguments (e.g.,
-Warray-bounds=@var{n}), so I'm leaving this as is.

>
> +@item -Wformat-length
> +@itemx -Wformat-length=@var{level}
> +@opindex Wformat-length
> +@opindex Wno-format-length
> +@opindex ffreestanding
> +@opindex fno-builtin
> +@opindex Wformat-length=
>
> This whole hunk is duplicated

So it is!  That's quite strange.  I'm not sure how it happened.
Maybe the same patch got applied twice.

>
> Usually, at the end of the option description, it is mentioned which
> options enabled this one (-Wformat=1 in this case, which is in turn
> enabled by -Wall).

I've added that.

>
> +The @option{-Wformat-length} option causes GCC to attempt to detect calls
> +to formatted functions such as @code{sprintf} that might overflow the
> +destination buffer, or bounded functions like @code{snprintf} that result
> +in output truncation.
>
> This text is a bit too verbose and quite different from other warning
> options. A humble proposal:
>
> Warn about calls to formatted functions, such as @code{sprintf}, that
> may overflow the destination buffer, or bounded functions, such as
> @code{snprintf} , that may? result in output truncation.
>
> +GCC counts the number of bytes that each format
> +string and directive within it writes into the provided buffer and, when
> +it detects that more bytes that fit in the destination buffer may be output,
> +it emits a warning.
>
> This is saying basically the same as the first sentence. Remove it?

Okay.

>
> +Directives whose arguments have values that can be
> +determined at compile-time account for the exact number of bytes they write.
> +Directives with arguments whose values cannot be determined are processed
> +based on heuristics that depend on the @var{level} argument to the option,
> +and on optimization.
>
> When the exact number of bytes written by a format string directive
> cannot be determined at compile-time, it is estimated based on
> heuristics that depend on optimization and the @var{level} argument.
> (I swapped the two because level is the next thing explained).

Okay.

>
> A better format (and it enables searching for specific levels) would be:

Thanks. I had not realized this.  I've restructured the text to fit
this format.  I was at first worried about what indenting the option
another level would look like in the PDF but it's not bad.

> +Enabling optimization will in most
> +cases improve the accuracy of the warning, although in some cases it may
> +also result in false positives.
>
> Since the example is not about optimization. Move this paragraph after
> the examples.

I found that moving this to the end of the first/main -Wformat-
length=level paragraph worked well.

> You will save some horizontal space, save the else branch and improve
> readability by doing:
>
> if (!val && fuzzy)
> {
> }
> if (!val)
>   return false;

Agreed.

>
> +/* Describes a location range outlining a substring within a string
> +   literal.  */
> +
> +class substring_loc
...
> +const char *
> +substring_loc::get_location (location_t *out_loc) const
> +{
> +  gcc_assert (out_loc);
> +
> +  if (!g_string_concat_db)
> +    g_string_concat_db
> +      = new (ggc_alloc <string_concat_db> ()) string_concat_db ();
> +
> +  static struct cpp_reader* parse_in;
> +  if (!parse_in)
> +    {
> +      /* Create and initialize a preprocessing reader.  */
> +      parse_in = cpp_create_reader (CLK_GNUC99, ident_hash, line_table);
> +      cpp_init_iconv (parse_in);
> +    }
> +
> +  return get_source_location_for_substring (parse_in, g_string_concat_db,
> +                        m_fmt_string_loc, CPP_STRING,
> +                        m_caret_idx, m_start_idx, m_end_idx,
> +                        out_loc);
> +}
> +
>
> Ideally, the diagnostics machinery would hide all these details from
> the middle-end. Surely, most of this can be shared with c-format.c.

I agree.  The challenge is that not all the bits this depends on
(the g_string_concat_db and parse_in globals defined in the front
end) are available in the middle end.  I've been talking to David
Malcolm about how best to factor things out of c-format.c and make
it available in both parts of the compiler under a convenient API.

>
> +/* Return the logarithm of X in BASE.  */
> +
> +static int
> +ilog (unsigned HOST_WIDE_INT x, int base)
> +{
> +  int res = 0;
> +  do {
> +    ++res;
> +    x /= base;
> +  } while (x);
> +  return res;
> +}
>
> Should this go to hwint.h ?

If there are other existing or potential users it would make sense
to make it more widely available.  I'll leave that to others with
a broader view of the code base to decide (my own rule of thumb is
to keep my patches contained and avoid touching parts I don't need
to).

Thanks again!
Martin

- improve sprintf buffer overflow detection (middle-end/49905)

Commit Message

Comments

Patch