Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(45)

Issue 135050043: code review 135050043: cmd/5l, cmd/6l, cmd/8l: fix nacl binary corruption bug (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by rsc
Modified:
9 years, 7 months ago
Reviewers:
gobot, iant, dvyukov
CC:
golang-codereviews
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : diff -r dbef39a08d10201cf67b38c16d223fdbc54ab385 https://code.google.com/p/go/ #

Patch Set 3 : diff -r d51c319e0f20b1306c07fcb80f643bc357ce3327 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -10 lines) Patch
M src/cmd/5l/asm.c View 1 1 chunk +2 lines, -2 lines 0 comments Download
M src/cmd/6l/asm.c View 1 2 chunks +4 lines, -4 lines 0 comments Download
M src/cmd/8l/asm.c View 1 1 chunk +4 lines, -4 lines 0 comments Download

Messages

Total messages: 6
rsc
Hello iant (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
9 years, 7 months ago (2014-08-28 02:53:26 UTC) #1
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=57dfd03985a9 *** cmd/5l, cmd/6l, cmd/8l: fix nacl binary corruption bug NaCl requires ...
9 years, 7 months ago (2014-08-28 02:53:32 UTC) #2
iant
LGTM
9 years, 7 months ago (2014-08-28 03:01:10 UTC) #3
gobot
This CL appears to have broken the netbsd-amd64-bsiegert builder. See http://build.golang.org/log/ee993e4b994d43bf8245ef70cce6f976db210235
9 years, 7 months ago (2014-08-28 03:33:56 UTC) #4
dvyukov
This looks suspiciously similar to what I observed here: https://code.google.com/p/go/issues/detail?id=8527 Any printfs fixed that crash ...
9 years, 7 months ago (2014-08-29 07:04:51 UTC) #5
rsc
9 years, 7 months ago (2014-08-29 14:10:41 UTC) #6
On Fri, Aug 29, 2014 at 3:04 AM, <dvyukov@google.com> wrote:

> This looks suspiciously similar to what I observed here:
>
> https://code.google.com/p/go/issues/detail?id=8527
>
> Any printfs fixed that crash as well.
> Can we consider the issue fixed?
>

Yes, that's the one. Fixed.

Russ
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b