Do not overflow string buffer (PR objc/85476).

Message ID 57b9fb88-7fa4-2fa7-1df4-754faaf53a9a@suse.cz
State New
Headers show
Series
  • Do not overflow string buffer (PR objc/85476).
Related show

Commit Message

Martin Liška April 20, 2018, 9:44 a.m.
Hi.

Quite obvious package that causes an ASAN error described in the PR.

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Ready to be installed?
Martin

gcc/objc/ChangeLog:

2018-04-20  Martin Liska  <mliska@suse.cz>

	PR objc/85476
	* objc-act.c (finish_class): Do not overflow string buffer.
---
 gcc/objc/objc-act.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jakub Jelinek April 20, 2018, 9:48 a.m. | #1
On Fri, Apr 20, 2018 at 11:44:35AM +0200, Martin Liška wrote:
> Hi.

> 

> Quite obvious package that causes an ASAN error described in the PR.

> 

> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

> 

> Ready to be installed?

> Martin

> 

> gcc/objc/ChangeLog:

> 

> 2018-04-20  Martin Liska  <mliska@suse.cz>

> 

> 	PR objc/85476

> 	* objc-act.c (finish_class): Do not overflow string buffer.


Ok, thanks.

	Jakub
Richard Biener April 20, 2018, 10:48 a.m. | #2
On Fri, Apr 20, 2018 at 11:44 AM, Martin Liška <mliska@suse.cz> wrote:
> Hi.

>

> Quite obvious package that causes an ASAN error described in the PR.

>

> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

>

> Ready to be installed?


Ok.

Richard.

> Martin

>

> gcc/objc/ChangeLog:

>

> 2018-04-20  Martin Liska  <mliska@suse.cz>

>

>         PR objc/85476

>         * objc-act.c (finish_class): Do not overflow string buffer.

> ---

>  gcc/objc/objc-act.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

>
Martin Sebor April 20, 2018, 3:58 p.m. | #3
On 04/20/2018 03:44 AM, Martin Liška wrote:
> Hi.

>

> Quite obvious package that causes an ASAN error described in the PR.

>

> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.


As an aside, I went and looked at the rest of code to see if
the overflow could be detected at compile time and if it could
be why it's not.  Here's what the code boils down to:

   void f (char*);

   void g (const char *s)
   {
      unsigned n = strlen (s);
      char *d = alloca (n);
      strcpy (d, s);
      f (d);
   }

Even though the off-by-one error is obvious it's not detected
either with _FORTIFY_SOURCE or without.  Both fail because
compute_builtin_object_size() only detects constant sizes.

But the strlen pass tracks both the size of allocations and
the lengths of even non-constant strings (computed by strlen)
so detecting the overflow there should be straightforward.
In the test case above the pass sees the following:

   _1 = __builtin_strlen (s_4(D));
   _9 = _1 & 4294967295;
   d_6 = __builtin_alloca (_9);
   __builtin_strcpy (d_6, s_4(D));

I've raised bug 85484 to try to implement this in GCC 9.

(Another way to handle this would be to enhance builtin-object
size to track non-constant sizes but that would require bigger
changes).

Martin

>

> Ready to be installed?

> Martin

>

> gcc/objc/ChangeLog:

>

> 2018-04-20  Martin Liska  <mliska@suse.cz>

>

> 	PR objc/85476

> 	* objc-act.c (finish_class): Do not overflow string buffer.

> ---

>  gcc/objc/objc-act.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

>

Patch

diff --git a/gcc/objc/objc-act.c b/gcc/objc/objc-act.c
index b87f7cc075e..d08693051ea 100644
--- a/gcc/objc/objc-act.c
+++ b/gcc/objc/objc-act.c
@@ -8003,7 +8003,7 @@  finish_class (tree klass)
 		    char *setter_name = (char *) alloca (length);
 		    tree ret_type, selector, arg_type, arg_name;
 
-		    strcpy (setter_name, full_setter_name);
+		    memcpy (setter_name, full_setter_name, length - 1);
 		    setter_name[length - 1] = '\0';
 		    ret_type = build_tree_list (NULL_TREE, void_type_node);
 		    arg_type = build_tree_list (NULL_TREE, TREE_TYPE (x));