diff mbox series

profile-count: Don't dump through a temporary buffer [PR111960]

Message ID ZdcOmBMaryuwCiTp@tucnak
State New
Headers show
Series profile-count: Don't dump through a temporary buffer [PR111960] | expand

Commit Message

Jakub Jelinek Feb. 22, 2024, 9:06 a.m. UTC 
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?

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  <jakub@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.


	Jakub

Comments

Richard Biener Feb. 22, 2024, 10:13 a.m. UTC | #1 
On Thu, Feb 22, 2024 at 10:07 AM Jakub Jelinek <jakub@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  <jakub@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
>
Jan Hubicka Feb. 22, 2024, 1:03 p.m. UTC | #2 
> 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?

Thanks for fixing it!
I believe the original reason why dump had the buffer argument was
pretty printing, which we do differently now.  Fully agree that the
fixed size buffer is ugly.

Honza
diff mbox series

Patch

--- 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