code review 151150044: [release-branch.go1.3] cmd/5l, cmd/6l, cmd/8l: fix nacl...

[release-branch.go1.3] cmd/5l, cmd/6l, cmd/8l: fix nacl binary corruption bug

««« CL 135050043 / 57dfd03985a9
cmd/5l, cmd/6l, cmd/8l: fix nacl binary corruption bug

NaCl requires the addition of a 32-byte "halt sled" at the end
of the text segment. This means that segtext.len is actually
32 bytes shorter than reality. The computation of the file offset
of the end of the data segment did not take this 32 bytes into
account, so if len and len+32 rounded up (by 64k) to different
values, the symbol table overwrote the last page of the data
segment.

The last page of the data segment is usually the C .string
symbols, which contain the strings used in error prints
by the runtime. So when this happens, your program
probably crashes, and then when it does, you get binary
garbage instead of all the usual prints.

The chance of hitting this with a randomly sized text segment
is 32 in 65536, or 1 in 2048.

If you add or remove ANY code while trying to debug this
problem, you're overwhelmingly likely to bump the text
segment one way or the other and make the bug disappear.

Correct all the computations to use segdata.fileoff+segdata.filelen
instead of trying to rederive segdata.fileoff.

This fixes the failure during the nacl/amd64p32 build.

TBR=iant
CC=golang-codereviews
https://codereview.appspot.com/135050043
»»»

[rsc, 2014-09-30 16:31:38]

Hello golang-codereviews@googlegroups.com (cc: adg, bradfitz, iant),

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

[bradfitz, 2014-09-30 16:37:34]

LGTM
 On Sep 30, 2014 9:31 AM, <rsc@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com (cc: adg, bradfitz, iant),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> [release-branch.go1.3] cmd/5l, cmd/6l, cmd/8l: fix nacl binary
> corruption bug
>
> ««« CL 135050043 / 57dfd03985a9
> cmd/5l, cmd/6l, cmd/8l: fix nacl binary corruption bug
>
> NaCl requires the addition of a 32-byte "halt sled" at the end
> of the text segment. This means that segtext.len is actually
> 32 bytes shorter than reality. The computation of the file offset
> of the end of the data segment did not take this 32 bytes into
> account, so if len and len+32 rounded up (by 64k) to different
> values, the symbol table overwrote the last page of the data
> segment.
>
> The last page of the data segment is usually the C .string
> symbols, which contain the strings used in error prints
> by the runtime. So when this happens, your program
> probably crashes, and then when it does, you get binary
> garbage instead of all the usual prints.
>
> The chance of hitting this with a randomly sized text segment
> is 32 in 65536, or 1 in 2048.
>
> If you add or remove ANY code while trying to debug this
> problem, you're overwhelmingly likely to bump the text
> segment one way or the other and make the bug disappear.
>
> Correct all the computations to use segdata.fileoff+segdata.filelen
> instead of trying to rederive segdata.fileoff.
>
> This fixes the failure during the nacl/amd64p32 build.
>
> TBR=iant
> CC=golang-codereviews
> https://codereview.appspot.com/135050043
> »»»
>
>
> Please review this at https://codereview.appspot.com/151150044/
>
> Affected files (+10, -10 lines):
>   M src/cmd/5l/asm.c
>   M src/cmd/6l/asm.c
>   M src/cmd/8l/asm.c
>
>
> Index: src/cmd/5l/asm.c
> ===================================================================
> --- a/src/cmd/5l/asm.c
> +++ b/src/cmd/5l/asm.c
> @@ -599,10 +599,10 @@
>                         if(iself)
>                                 goto ElfSym;
>                 case Hplan9:
> -                       symo = HEADR+segtext.len+segdata.filelen;
> +                       symo = segdata.fileoff+segdata.filelen;
>                         break;
>                 ElfSym:
> -                       symo = rnd(HEADR+segtext.filelen,
> INITRND)+rnd(HEADR+segrodata.filelen, INITRND)+segdata.filelen;
> +                       symo = segdata.fileoff+segdata.filelen;
>                         symo = rnd(symo, INITRND);
>                         break;
>                 }
> Index: src/cmd/6l/asm.c
> ===================================================================
> --- a/src/cmd/6l/asm.c
> +++ b/src/cmd/6l/asm.c
> @@ -689,10 +689,10 @@
>                 case Hplan9:
>                 case Helf:
>                         debug['s'] = 1;
> -                       symo = HEADR+segtext.len+segdata.filelen;
> +                       symo = segdata.fileoff+segdata.filelen;
>                         break;
>                 case Hdarwin:
> -                       symo = rnd(HEADR+segtext.len,
> INITRND)+rnd(segdata.filelen, INITRND)+machlink;
> +                       symo = segdata.fileoff+rnd(segdata.filelen,
> INITRND)+machlink;
>                         break;
>                 case Hlinux:
>                 case Hfreebsd:
> @@ -701,11 +701,11 @@
>                 case Hdragonfly:
>                 case Hsolaris:
>                 case Hnacl:
> -                       symo = rnd(HEADR+segtext.len,
> INITRND)+rnd(segrodata.len, INITRND)+segdata.filelen;
> +                       symo = segdata.fileoff+segdata.filelen;
>                         symo = rnd(symo, INITRND);
>                         break;
>                 case Hwindows:
> -                       symo = rnd(HEADR+segtext.filelen,
> PEFILEALIGN)+segdata.filelen;
> +                       symo = segdata.fileoff+segdata.filelen;
>                         symo = rnd(symo, PEFILEALIGN);
>                         break;
>                 }
> Index: src/cmd/8l/asm.c
> ===================================================================
> --- a/src/cmd/8l/asm.c
> +++ b/src/cmd/8l/asm.c
> @@ -619,17 +619,17 @@
>                         if(iself)
>                                 goto Elfsym;
>                 case Hplan9:
> -                       symo = HEADR+segtext.filelen+segdata.filelen;
> +                       symo = segdata.fileoff+segdata.filelen;
>                         break;
>                 case Hdarwin:
> -                       symo = rnd(HEADR+segtext.filelen,
> INITRND)+rnd(segdata.filelen, INITRND)+machlink;
> +                       symo = segdata.fileoff+rnd(segdata.filelen,
> INITRND)+machlink;
>                         break;
>                 Elfsym:
> -                       symo = rnd(HEADR+segtext.filelen,
> INITRND)+rnd(HEADR+segrodata.filelen, INITRND)+segdata.filelen;
> +                       symo = segdata.fileoff+segdata.filelen;
>                         symo = rnd(symo, INITRND);
>                         break;
>                 case Hwindows:
> -                       symo = rnd(HEADR+segtext.filelen,
> PEFILEALIGN)+segdata.filelen;
> +                       symo = segdata.fileoff+segdata.filelen;
>                         symo = rnd(symo, PEFILEALIGN);
>                         break;
>                 }
>
>
>

[rsc, 2014-09-30 16:43:53]

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

[release-branch.go1.3] cmd/5l, cmd/6l, cmd/8l: fix nacl binary corruption bug

««« CL 135050043 / 57dfd03985a9
cmd/5l, cmd/6l, cmd/8l: fix nacl binary corruption bug

NaCl requires the addition of a 32-byte "halt sled" at the end
of the text segment. This means that segtext.len is actually
32 bytes shorter than reality. The computation of the file offset
of the end of the data segment did not take this 32 bytes into
account, so if len and len+32 rounded up (by 64k) to different
values, the symbol table overwrote the last page of the data
segment.

The last page of the data segment is usually the C .string
symbols, which contain the strings used in error prints
by the runtime. So when this happens, your program
probably crashes, and then when it does, you get binary
garbage instead of all the usual prints.

The chance of hitting this with a randomly sized text segment
is 32 in 65536, or 1 in 2048.

If you add or remove ANY code while trying to debug this
problem, you're overwhelmingly likely to bump the text
segment one way or the other and make the bug disappear.

Correct all the computations to use segdata.fileoff+segdata.filelen
instead of trying to rederive segdata.fileoff.

This fixes the failure during the nacl/amd64p32 build.

TBR=iant
CC=golang-codereviews
https://codereview.appspot.com/135050043
»»»

LGTM=bradfitz
R=golang-codereviews
CC=adg, bradfitz, golang-codereviews, iant
https://codereview.appspot.com/151150044

[gobot, 2014-09-30 17:57:11]

This CL appears to have broken the netbsd-amd64-bsiegert builder.
See http://build.golang.org/log/a18b28112d6c2c69457cd9f5fe61615626be86d3