diff mbox series

diagnostic, pch: Fix up the new diagnostic PCH methods for ubsan checking [PR116936]

Message ID Zv+9bKdvzS7N6o8S@tucnak
State New
Headers show
Series diagnostic, pch: Fix up the new diagnostic PCH methods for ubsan checking [PR116936] | expand

Commit Message

Jakub Jelinek Oct. 4, 2024, 10:03 a.m. UTC 
Hi!

The PR notes that the new pch_save/pch_restore methods I've added
recently invoke UB if either m_classification_history.address ()
or m_push_list.address () is NULL (which can happen if those vectors
are empty (and in the pch_save case nothing has been pushed into them
before either).  While the corresponding length is necessarily 0,
fwrite (NULL, something, 0, f) or
fread (NULL, something, 0, f) still invoke UB.

The following patch fixes that by not calling fwrite/fread if the
corresponding length is 0.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2024-10-04  Jakub Jelinek  <jakub@redhat.com>

	PR pch/116936
	* diagnostic.cc (diagnostic_option_classifier::pch_save): Only call
	fwrite if corresponding length is non-zero.
	(diagnostic_option_classifier::pch_restore): Only call fread if
	corresponding length is non-zero.


	Jakub

Comments

Richard Biener Oct. 4, 2024, 11:41 a.m. UTC | #1 
On Fri, Oct 4, 2024 at 12:04 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> The PR notes that the new pch_save/pch_restore methods I've added
> recently invoke UB if either m_classification_history.address ()
> or m_push_list.address () is NULL (which can happen if those vectors
> are empty (and in the pch_save case nothing has been pushed into them
> before either).  While the corresponding length is necessarily 0,
> fwrite (NULL, something, 0, f) or
> fread (NULL, something, 0, f) still invoke UB.
>
> The following patch fixes that by not calling fwrite/fread if the
> corresponding length is 0.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

> 2024-10-04  Jakub Jelinek  <jakub@redhat.com>
>
>         PR pch/116936
>         * diagnostic.cc (diagnostic_option_classifier::pch_save): Only call
>         fwrite if corresponding length is non-zero.
>         (diagnostic_option_classifier::pch_restore): Only call fread if
>         corresponding length is non-zero.
>
> --- gcc/diagnostic.cc.jj        2024-10-01 09:38:58.014961851 +0200
> +++ gcc/diagnostic.cc   2024-10-02 20:33:37.922953272 +0200
> @@ -165,11 +165,13 @@ diagnostic_option_classifier::pch_save (
>    unsigned int lengths[2] = { m_classification_history.length (),
>                               m_push_list.length () };
>    if (fwrite (lengths, sizeof (lengths), 1, f) != 1
> -      || fwrite (m_classification_history.address (),
> -                sizeof (diagnostic_classification_change_t),
> -                lengths[0], f) != lengths[0]
> -      || fwrite (m_push_list.address (), sizeof (int),
> -                lengths[1], f) != lengths[1])
> +      || (lengths[0]
> +         && fwrite (m_classification_history.address (),
> +                    sizeof (diagnostic_classification_change_t),
> +                    lengths[0], f) != lengths[0])
> +      || (lengths[1]
> +         && fwrite (m_push_list.address (), sizeof (int),
> +                    lengths[1], f) != lengths[1]))
>      return -1;
>    return 0;
>  }
> @@ -187,11 +189,13 @@ diagnostic_option_classifier::pch_restor
>    gcc_checking_assert (m_push_list.is_empty ());
>    m_classification_history.safe_grow (lengths[0]);
>    m_push_list.safe_grow (lengths[1]);
> -  if (fread (m_classification_history.address (),
> -            sizeof (diagnostic_classification_change_t),
> -            lengths[0], f) != lengths[0]
> -      || fread (m_push_list.address (), sizeof (int),
> -               lengths[1], f) != lengths[1])
> +  if ((lengths[0]
> +       && fread (m_classification_history.address (),
> +                sizeof (diagnostic_classification_change_t),
> +                lengths[0], f) != lengths[0])
> +      || (lengths[1]
> +         && fread (m_push_list.address (), sizeof (int),
> +                   lengths[1], f) != lengths[1]))
>      return -1;
>    return 0;
>  }
>
>         Jakub
>
diff mbox series

Patch

--- gcc/diagnostic.cc.jj	2024-10-01 09:38:58.014961851 +0200
+++ gcc/diagnostic.cc	2024-10-02 20:33:37.922953272 +0200
@@ -165,11 +165,13 @@  diagnostic_option_classifier::pch_save (
   unsigned int lengths[2] = { m_classification_history.length (),
 			      m_push_list.length () };
   if (fwrite (lengths, sizeof (lengths), 1, f) != 1
-      || fwrite (m_classification_history.address (),
-		 sizeof (diagnostic_classification_change_t),
-		 lengths[0], f) != lengths[0]
-      || fwrite (m_push_list.address (), sizeof (int),
-		 lengths[1], f) != lengths[1])
+      || (lengths[0]
+	  && fwrite (m_classification_history.address (),
+		     sizeof (diagnostic_classification_change_t),
+		     lengths[0], f) != lengths[0])
+      || (lengths[1]
+	  && fwrite (m_push_list.address (), sizeof (int),
+		     lengths[1], f) != lengths[1]))
     return -1;
   return 0;
 }
@@ -187,11 +189,13 @@  diagnostic_option_classifier::pch_restor
   gcc_checking_assert (m_push_list.is_empty ());
   m_classification_history.safe_grow (lengths[0]);
   m_push_list.safe_grow (lengths[1]);
-  if (fread (m_classification_history.address (),
-	     sizeof (diagnostic_classification_change_t),
-	     lengths[0], f) != lengths[0]
-      || fread (m_push_list.address (), sizeof (int),
-		lengths[1], f) != lengths[1])
+  if ((lengths[0]
+       && fread (m_classification_history.address (),
+		 sizeof (diagnostic_classification_change_t),
+		 lengths[0], f) != lengths[0])
+      || (lengths[1]
+	  && fread (m_push_list.address (), sizeof (int),
+		    lengths[1], f) != lengths[1]))
     return -1;
   return 0;
 }