x/crypto/poly1305: crash on ARM #17499
Comments
|
Hmm, looks like this might be down to my suggestion to use |
|
@mundaym, yes, NOFRAME is only implemented on ppc64 and s390x, and on amd64 for dropping the frame pointer. |
|
Yeah, in this file: x/crypto/poly1305/sum_arm.s |
|
Oh I see. It's in a subrepo. I am not sure I understand the code: it pushes may registers to stack, without allocating a stack frame. Does the caller allocate frame for it? How could (accidentally) saving LR to stack in |
|
To clarify, NOFRAME is no-op. |
|
OK, thanks Cherry, so it sounds like this probably isn't a regression due to the NOFRAME thing (although that should be fixed). In that case I suspect that |
Probably not, depending on whether saving LR causes a problem (not likely).
Yes, I think so.
Pointers are fine because its call tree is NOSPLIT, i.e. once it enters the function it is not preemptible. But I'm worried that the decrements of SP (in many places) may overflow the stack. That said, I'm not familiar with that code. There may be some thoughts behind it that I missed. |
|
I know the original author of the assembly code (which was not written with Go in mind) and I suspect that it was correct at the time. I don't know the person who originally converted it to Go however, and I suspect that I'll need some assistance in fixing this. The only entry point for the functions in Building a binary and running (Recall that the This is stack check, I assume, and it's checking for 284 bytes of free space. If the stack isn't large enough, it'll end up back here via This has been added by the compiler, although I'm not sure why. This is the loading of argument from the stack, but the offsets assume that R13 has already been moved down ~280 bytes, although I don't know when that happened. This will subtract between 64 and 127 bytes from the stack pointer and end up aligning it to 64 bytes. The rest of the code here operates on a datastructure that's ~64 bytes and which will be placed at this point in the stack. It doesn't help that I don't understand ARM assembly (instructions like The code does keep pointers into the stack in registers however. But there are no other stack-splitting points after the first. Can the GC preempt arbitrarily, or only at stack splits? |
When you declare frame size in It is very rare that hand written assembly decrement SP, probably only in code that manipulates stack frame.
Because it decremented SP first.
This, as well as other
Only on preemption points, i.e. stack splits. So this is fine. |
|
Thank you for that. It seems that this assembly code has been working only because of luck for a while. I can take a shot at fixing but but there's still something that I don't understand:
I assumed that this wrote the contents of R14 to the address (R13 - 0x11c). But your comments suggest that it also updates the value of R13? Is that the case? Does it actually set R13 = R13 - 0x11c and then do a write? (Probably I should try and setup an ARM environment with qemu or something but I currently don't have anything like that.) |
|
Yes, it does both |
|
CL https://golang.org/cl/31443 mentions this issue. |
|
CL https://golang.org/cl/31592 mentions this issue. |
This change updates the vendored copy of x/crypto/poly1305, specifically to include the following changes: 3ded668 poly1305: enable assembly for ARM in Go 1.6. dec8741 poly1305: fix stack handling in sum_arm.s Fixes #17499. Change-Id: I8f152da9599bd15bb976f630b0ef602be05143d3 Reviewed-on: https://go-review.googlesource.com/31592 Run-TryBot: Adam Langley <agl@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Up till now, sum_arm.s was working only because of luck. It was written assuming that it had stack space below the current stack pointer, but Go decrements the stack pointer in the function prelude, so it was just writing off the end of the stack. This change fixes the stack manipulation so that it only writes within the bounds. Fixes golang/go#17499. Change-Id: I1951b3344c21f6bd6ade79da8b96dd1bb68180db Reviewed-on: https://go-review.googlesource.com/31443 Reviewed-by: Cherry Zhang <cherryyz@google.com> Run-TryBot: Cherry Zhang <cherryyz@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
Up till now, sum_arm.s was working only because of luck. It was written assuming that it had stack space below the current stack pointer, but Go decrements the stack pointer in the function prelude, so it was just writing off the end of the stack. This change fixes the stack manipulation so that it only writes within the bounds. Fixes golang/go#17499. Change-Id: I1951b3344c21f6bd6ade79da8b96dd1bb68180db Reviewed-on: https://go-review.googlesource.com/31443 Reviewed-by: Cherry Zhang <cherryyz@google.com> Run-TryBot: Cherry Zhang <cherryyz@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
Up till now, sum_arm.s was working only because of luck. It was written assuming that it had stack space below the current stack pointer, but Go decrements the stack pointer in the function prelude, so it was just writing off the end of the stack. This change fixes the stack manipulation so that it only writes within the bounds. Fixes golang/go#17499. Change-Id: I1951b3344c21f6bd6ade79da8b96dd1bb68180db Reviewed-on: https://go-review.googlesource.com/31443 Reviewed-by: Cherry Zhang <cherryyz@google.com> Run-TryBot: Cherry Zhang <cherryyz@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
Up till now, sum_arm.s was working only because of luck. It was written assuming that it had stack space below the current stack pointer, but Go decrements the stack pointer in the function prelude, so it was just writing off the end of the stack. This change fixes the stack manipulation so that it only writes within the bounds. Fixes golang/go#17499. Change-Id: I1951b3344c21f6bd6ade79da8b96dd1bb68180db Reviewed-on: https://go-review.googlesource.com/31443 Reviewed-by: Cherry Zhang <cherryyz@google.com> Run-TryBot: Cherry Zhang <cherryyz@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
Noticed a trybot flake on ARM:
https://storage.googleapis.com/go-build-log/fbb1da3a/linux-arm_5dfc64f9.log
The text was updated successfully, but these errors were encountered: