runtime: fix badsignal2 to initialize r3 with a valid address #50822
Conversation
This comment has been minimized.
This comment has been minimized.
|
This PR (HEAD: 1a53732) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/381196 to see it. Tip: You can toggle comments from me using the |
|
Message from Gopher Robot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/381196. |
|
Message from Michael Knyszek: Patch Set 1: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/381196. |
|
Message from Michael Knyszek: Patch Set 1: Run-TryBot+1 Trust+1 (3 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/381196. |
|
Message from Gopher Robot: Patch Set 1: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/381196. |
|
Message from Gopher Robot: Patch Set 1: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/381196. |
|
Message from Michael Knyszek: Patch Set 1: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/381196. |
|
Message from Gopher Robot: Patch Set 1: TryBot-Result-1 (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/381196. |
marler8997
force-pushed
the
arm64Badsignal
branch
from
1a53732 to
a4f0380
Compare
|
This PR (HEAD: a4f0380) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/381196 to see it. Tip: You can toggle comments from me using the |
marler8997
force-pushed
the
arm64Badsignal
branch
from
a4f0380 to
8d0948c
Compare
|
This PR (HEAD: 8d0948c) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/381196 to see it. Tip: You can toggle comments from me using the |
We ran into an issue on the hololens which is an arm64 windows platform. We noticed that in runtime.badsignal2 it would call WriteFile, but then we would get an Access Violation inside it when it attempted to derefence the lpNumberOfBytesWritten variable. It appears that runtime.badsignal2 is assigning this variable, which is a pointer, to whatever is in r13 (note this is assembly), but I don't see where r13 is initialized and in our case it was set to 1, so WriteFile ended up dereferencing the value 1 causing the Access Violation. This change initially modified the assembly to set r3 to 0 which would fix the issue by allowing WriteFile to print the error message and then return, however, according to Microsoft's docs, lpNumberOfBytesWritten should only be assigned "NULL" if the overlapped argumet is not NULL, which is not the case here. So I've updated the assembly code to set R3 to a location on the stack. Note that it's likely this issue hasn't been noticed because it occurs right before an abort so the result ends up being almost the same except the error message doesn't get printed to stderr. Also this would sometimes work because whatever happens to be in r13 could be 0 or a valid pointer.
marler8997
force-pushed
the
arm64Badsignal
branch
from
8d0948c to
f8fdf74
Compare
|
This PR (HEAD: f8fdf74) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/381196 to see it. Tip: You can toggle comments from me using the |
marler8997
changed the title
|
Message from Jonathan Marler: Patch Set 5: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/381196. |
|
Message from Jonathan Marler: Patch Set 5: Code-Review+1 (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/381196. |
|
Message from Michael Pratt: Patch Set 5: Code-Review+1 (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/381196. |
|
Message from Michael Knyszek: Patch Set 5: Code-Review+2 Please don’t reply on this GitHub thread. Visit golang.org/cl/381196. |
We ran into an issue on the hololens which is an arm64 windows platform.
We noticed that in runtime.badsignal2 it would call WriteFile, but then
we would get an Access Violation inside it when it attempted to derefence
the lpNumberOfBytesWritten variable. It appears that runtime.badsignal2
is assigning this variable, which is a pointer, to whatever is in r13
(note this is assembly), but I don't see where r13 is initialized and in
our case it was set to 1, so WriteFile ended up dereferencing the value 1
causing the Access Violation.
This change initially modified the assembly to set r3 to 0 which would
fix the issue by allowing WriteFile to print the error message and then
return, however, according to Microsoft's docs, lpNumberOfBytesWritten
should only be assigned "NULL" if the overlapped argumet is not NULL, which
is not the case here. So I've updated the assembly code to set R3 to a
location on the stack.
Note that it's likely this issue hasn't been noticed because it occurs
right before an abort so the result ends up being almost the same except
the error message doesn't get printed to stderr. Also this would sometimes
work because whatever happens to be in r13 could be 0 or a valid pointer.