On Thu, Feb 22, 2024 at 10:07 AM Jakub Jelinek <ja...@redhat.com> wrote:
>
> Hi!
>
> The profile_count::dump (char *, struct function * = NULL) const;
> method has a single caller, the
> profile_count::dump (FILE *f, struct function *fun) const;
> method and for that going through a temporary buffer is just slower
> and opens doors for buffer overflows, which is exactly why this P1
> was filed.
> The buffer size is 64 bytes, the previous maximum
> "%" PRId64 " (%s)"
> would print up to 61 bytes in there (19 bytes for arbitrary uint64_t:61
> bitfield printed as signed, "estimated locally, globally 0 adjusted"
> i.e. 38 bytes longest %s and 4 other characters).
> Now, after the r14-2389 changes, it can be
> 19 + 38 plus 11 other characters + %.4f, which is worst case
> 309 chars before decimal point, decimal point and 4 digits after it,
> so total 382 bytes.
>
> So, either we could bump the buffer[64] to buffer[400], or the following
> patch just drops the indirection through buffer and prints it directly to
> stream. After all, having APIs which fill in some buffer without passing
> down the size of the buffer is just asking for buffer overflows over time.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
Richard.
> Or do you want buffer[400]; instead? Or buffer[128]; and somehow prevent
> arbitrarily long doubles? Or add size_t next to char * arguments and use
> snprintf? Though, truncated messages would look ugly.
>
> 2024-02-22 Jakub Jelinek <ja...@redhat.com>
>
> PR ipa/111960
> * profile-count.h (profile_count::dump): Remove overload with
> char * first argument.
> * profile-count.cc (profile_count::dump): Change overload with char *
> first argument which uses sprintf into the overfload with FILE *
> first argument and use fprintf instead. Remove overload which wrapped
> it.
>
> --- gcc/profile-count.h.jj 2024-01-03 11:51:30.309748150 +0100
> +++ gcc/profile-count.h 2024-02-21 21:04:22.338905728 +0100
> @@ -1299,9 +1299,6 @@ public:
> /* Output THIS to F. */
> void dump (FILE *f, struct function *fun = NULL) const;
>
> - /* Output THIS to BUFFER. */
> - void dump (char *buffer, struct function *fun = NULL) const;
> -
> /* Print THIS to stderr. */
> void debug () const;
>
> --- gcc/profile-count.cc.jj 2024-01-03 11:51:40.782602796 +0100
> +++ gcc/profile-count.cc 2024-02-21 21:05:28.521994913 +0100
> @@ -84,34 +84,24 @@ const char *profile_quality_display_name
> "precise"
> };
>
> -/* Dump THIS to BUFFER. */
> +/* Dump THIS to F. */
>
> void
> -profile_count::dump (char *buffer, struct function *fun) const
> +profile_count::dump (FILE *f, struct function *fun) const
> {
> if (!initialized_p ())
> - sprintf (buffer, "uninitialized");
> + fprintf (f, "uninitialized");
> else if (fun && initialized_p ()
> && fun->cfg
> && ENTRY_BLOCK_PTR_FOR_FN (fun)->count.initialized_p ())
> - sprintf (buffer, "%" PRId64 " (%s, freq %.4f)", m_val,
> + fprintf (f, "%" PRId64 " (%s, freq %.4f)", m_val,
> profile_quality_display_names[m_quality],
> to_sreal_scale (ENTRY_BLOCK_PTR_FOR_FN (fun)->count).to_double
> ());
> else
> - sprintf (buffer, "%" PRId64 " (%s)", m_val,
> + fprintf (f, "%" PRId64 " (%s)", m_val,
> profile_quality_display_names[m_quality]);
> }
>
> -/* Dump THIS to F. */
> -
> -void
> -profile_count::dump (FILE *f, struct function *fun) const
> -{
> - char buffer[64];
> - dump (buffer, fun);
> - fputs (buffer, f);
> -}
> -
> /* Dump THIS to stderr. */
>
> void
>
> Jakub
>
