code review 7838050: cmd/gc: fix nil pointer dereferences.

cmd/gc: fix nil pointer dereferences.

Fixes issue 5119.

[remyoudompheng, 2013-03-24 09:52:50]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dvyukov, 2013-03-24 10:12:43]

I am not sure it worth fixing, because gc setups own SIGSEGV  andler
that does exit(1) if there were some compilation errors before.

On Sun, Mar 24, 2013 at 1:52 PM,  <remyoudompheng@gmail.com> wrote:
> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> cmd/gc: fix nil pointer dereferences.
>
> Fixes issue 5119.
>
> Please review this at https://codereview.appspot.com/7838050/
>
> Affected files:
>   M src/cmd/gc/dcl.c
>   M src/cmd/gc/subr.c
>
>
> Index: src/cmd/gc/dcl.c
> ===================================================================
> --- a/src/cmd/gc/dcl.c
> +++ b/src/cmd/gc/dcl.c
> @@ -1339,6 +1339,8 @@
>         f = methtype(pa, 1);
>         if(f == T) {
>                 t = pa;
> +               if(t == T) // rely on typecheck having complained before
> +                       return;
>                 if(t != T) {
>                         if(isptr[t->etype]) {
>                                 if(t->sym != S) {
> @@ -1347,10 +1349,8 @@
>                                 }
>                                 t = t->type;
>                         }
> -               }
> -               if(t->broke) // rely on typecheck having complained before
> -                       return;
> -               if(t != T) {
> +                       if(t->broke) // rely on typecheck having complained
> before
> +                               return;
>                         if(t->sym == S) {
>                                 yyerror("invalid receiver type %T (%T is an
> unnamed type)", pa, t);
>                                 return;
> Index: src/cmd/gc/subr.c
> ===================================================================
> --- a/src/cmd/gc/subr.c
> +++ b/src/cmd/gc/subr.c
> @@ -714,6 +714,12 @@
>
>         a = *(Type**)va;
>         b = *(Type**)vb;
> +       if(a->sym == S && b->sym == S)
> +               return 0;
> +       if(a->sym == S)
> +               return -1;
> +       if(b->sym == S)
> +               return 1;
>         i = strcmp(a->sym->name, b->sym->name);
>         if(i != 0)
>                 return i;
> @@ -1393,7 +1399,7 @@
>         Node *r, *old;
>         char *why;
>
> -       if(n == N || n->type == T)
> +       if(n == N || n->type == T || n->type->broke)
>                 return n;
>
>         old = n;
>
>
> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[dave_cheney.net, 2013-03-25 02:49:42]

One small suggestion.

@dmitry: I think we should try to address any complaint that the tools find, so
that we can avoid being distracted by them later on.

https://codereview.appspot.com/7838050/diff/5001/src/cmd/gc/dcl.c
File src/cmd/gc/dcl.c (right):

https://codereview.appspot.com/7838050/diff/5001/src/cmd/gc/dcl.c#newcode1344
src/cmd/gc/dcl.c:1344: if(t != T) {
Do you need this check? The line above ensures if t == T 1344 won't be reached.

[dvyukov, 2013-03-25 04:05:36]

On Mon, Mar 25, 2013 at 6:49 AM,  <dave@cheney.net> wrote:
> One small suggestion.
>
> @dmitry: I think we should try to address any complaint that the tools
> find, so that we can avoid being distracted by them later on.

I think for ASan SIGSEGV complaining just must be turned off.

[rsc, 2013-03-25 21:57:54]

LGTM

The SIGSEGV handler is there to give a nicer error, but it's still good to
avoid nil derefs. Thanks.

[remyoudompheng, 2013-03-26 07:20:17]

*** Submitted as https://code.google.com/p/go/source/detail?r=924fbbd725d4 ***

cmd/gc: fix nil pointer dereferences.

Fixes issue 5119.

R=golang-dev, dvyukov, dave, rsc
CC=golang-dev
https://codereview.appspot.com/7838050