code review 60740044: cmd/gc: fix nil pointer dereference

cmd/gc: fix nil pointer dereference

[0intro, 2014-02-06 22:40:08]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[dave_cheney.net, 2014-02-06 22:43:59]

Thanks. The fix looks pretty straight forward. Is there an issue or a test
that can assert we don't revert this in the future ?


On Fri, Feb 7, 2014 at 9:40 AM, <0intro@gmail.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> cmd/gc: fix nil pointer dereference
>
> Please review this at https://codereview.appspot.com/60740044/
>
> Affected files (+1, -1 lines):
>   M src/cmd/gc/typecheck.c
>
>
> Index: src/cmd/gc/typecheck.c
> ===================================================================
> --- a/src/cmd/gc/typecheck.c
> +++ b/src/cmd/gc/typecheck.c
> @@ -3199,7 +3199,7 @@
>                 n->type->sym = n->sym;
>                 nerrors0 = nerrors;
>                 typecheckdeftype(n);
> -               if(n->type->etype == TFORW && nerrors > nerrors0) {
> +               if(n->type != T && n->type->etype == TFORW && nerrors >
> nerrors0) {
>                         // Something went wrong during type-checking,
>                         // but it was reported. Silence future errors.
>                         n->type->broke = 1;
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[0intro, 2014-02-06 22:51:06]

> Thanks. The fix looks pretty straight forward. Is there an issue or a
> test that can assert we don't revert this in the future ?

I don't think there is any specific test. I've noticed this issue
because 8g was crashing when executing some tests on Plan 9.

For example:

# go run run.go -- fixedbugs/issue4359.go

issue4359.go:13: missing error "undefined"
Unmatched Errors:
8g 219077: suicide: sys: trap: fault read addr=0x0 pc=0x00032562

FAIL	fixedbugs/issue4359.go	0.100s

-- 
David du Colombier

[dave_cheney.net, 2014-02-06 22:54:07]

Fair enough, looks like a test exists already. Please wait for Ian, Remy or
rsc.


On Fri, Feb 7, 2014 at 9:50 AM, David du Colombier <0intro@gmail.com> wrote:

> > Thanks. The fix looks pretty straight forward. Is there an issue or a
> > test that can assert we don't revert this in the future ?
>
> I don't think there is any specific test. I've noticed this issue
> because 8g was crashing when executing some tests on Plan 9.
>
> For example:
>
> # go run run.go -- fixedbugs/issue4359.go
>
> issue4359.go:13: missing error "undefined"
> Unmatched Errors:
> 8g 219077: suicide: sys: trap: fault read addr=0x0 pc=0x00032562
>
> FAIL    fixedbugs/issue4359.go  0.100s
>
> --
> David du Colombier
>

[0intro, 2014-02-07 08:09:52]

> This fix looks fine to me but in the long run we
> should probably setup a note handler like the one
> for Unix systems (see src/cmd/gc/lex.c:/^fault).

Yes. I think it will be needed to catch the floating point and other
exceptions on Plan 9.
I've started to write the code but I've not tested it yet.

-- 
David du Colombier

[iant, 2014-02-07 14:14:26]

LGTM

[0intro, 2014-02-07 14:43:51]

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

cmd/gc: fix nil pointer dereference

LGTM=iant
R=golang-codereviews, dave, iant
CC=golang-codereviews
https://codereview.appspot.com/60740044

[gobot, 2014-02-07 14:47:54]

This CL appears to have broken the darwin-386-cheney builder.

[0intro, 2014-02-07 15:28:29]

> This CL appears to have broken the darwin-386-cheney builder.
>
> https://codereview.appspot.com/60740044/

Sorry, I've introduced a regression. I've submitted CL 60760046 to fix it.

-- 
David du Colombier

[0intro, 2014-02-07 17:04:45]

This change seems to work around the issue, but I'm wondering why
n->type == T when running some tests like fixedbugs/issue5581.go.

--- a/src/cmd/gc/typecheck.c
+++ b/src/cmd/gc/typecheck.c
@@ -3196,6 +3196,8 @@
                        defercheckwidth();
                n->walkdef = 1;
                n->type = typ(TFORW);
+               if(n->type == T)
+                       break;
                n->type->sym = n->sym;
                nerrors0 = nerrors;
                typecheckdeftype(n);

-- 
David du Colombier

[iant, 2014-02-07 18:36:41]

On Fri, Feb 7, 2014 at 9:04 AM, David du Colombier <0intro@gmail.com> wrote:
> This change seems to work around the issue, but I'm wondering why
> n->type == T when running some tests like fixedbugs/issue5581.go.
>
> --- a/src/cmd/gc/typecheck.c
> +++ b/src/cmd/gc/typecheck.c
> @@ -3196,6 +3196,8 @@
>                         defercheckwidth();
>                 n->walkdef = 1;
>                 n->type = typ(TFORW);
> +               if(n->type == T)
> +                       break;
>                 n->type->sym = n->sym;
>                 nerrors0 = nerrors;
>                 typecheckdeftype(n);


I don't understand that at all.  typ is a simple function in subr.c
that always returns the result of mal.  T is defined to 0 in go.h.
mal can never return 0.  I don't see how that condition could ever be
true.

Ian

[0intro, 2014-02-07 19:04:18]

Sorry, I've got confused. n->ntype->type == T, thus typecheckdeftype()
set n->type = T. Hence the dereferencing and the caught SIGSEGV.

I don't really understand how the check preventing the "invalid
recursive type" error is supposed to work
(http://code.google.com/p/go/source/detail?r=46750f5e7801).

-- 
David du Colombier

[0intro, 2014-02-07 22:52:00]

As an experiment, I did the following change, which
I believe may be closer to the expected behavior,
as far as I understand it.

--- a/src/cmd/gc/typecheck.c
+++ b/src/cmd/gc/typecheck.c
@@ -3009,7 +3009,7 @@
 static void
 typecheckdeftype(Node *n)
 {
-       int lno;
+       int lno, nerrors0;
        Type *t;
        NodeList *l;
 
@@ -3018,10 +3018,12 @@
        setlineno(n);
        n->type->sym = n->sym;
        n->typecheck = 1;
+       nerrors0 = nerrors;
        typecheck(&n->ntype, Etype);
        if((t = n->ntype->type) == T) {
                n->diag = 1;
-               n->type = T;
+               if (nerrors == nerrors0)
+                       n->type = T;
                goto ret;
        }
        if(n->type == T) {

With this change, 6g return the following errors:

% /usr/local/go/pkg/tool/linux_amd64/6g fixedbugs/issue5581.go
fixedbugs/issue5581.go:18: y.A undefined (type *Bar has no field or method A)
fixedbugs/issue5581.go:29: undefined: Blah

Previously, the output was:

% /usr/local/go/pkg/tool/linux_amd64/6g fixedbugs/issue5581.go
fixedbugs/issue5581.go:29: undefined: Blah

Where the this error is followed by a SIGSEGV caught in cmd/gc/lex.c:/^main.

And before Russ's change, the output was :

% /usr/local/go/pkg/tool/linux_amd64/6g fixedbugs/issue5581.go
fixedbugs/issue5581.go:14: invalid recursive type Bar
fixedbugs/issue5581.go:18: y.A undefined (type *Bar has no field or method A)
fixedbugs/issue5581.go:29: undefined: Blah

Where the "invalid recursive type Bar" error was incorrect.

Does it make sense? What could be a proper way to fix it?

Thanks.

-- 
David du Colombier