1478623 - mpi_arm.c functions yield miscompilations when inlined by LTO

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Mike Hommey [:glandium]]

Bug 1477929 was an obvious build error, this one was only detected because it made Firefox crash at runtime. Details will be in the patch commit message.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Add a r/w constraint to a_len in all asm blocks in mpi_arm.c

While bug 1477929 fixed the obvious build failure, it still allowed the
compiler to break things when it inlines the mpi_arm.c functions into
its callers via LTO.

The problem is that all those assembly blocks take a length as input in
a register, and decrement that register. But the constraints are not
explicit about those writes to the register, so the compiler may decide
to reuse them as the original value for the length in code following the
inlined code. It actually happily does so, which leads to interesting
crashes.

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8995164 [details] [diff] [review]
Add a r/w constraint to a_len in all asm blocks in mpi_arm.c

Looks like this is not enough. Many Android tests still fail with this patch, while they don't if I add __attribute__((noinline)) to all these functions.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Add r/w constraints to modified registers to asm blocks in mpi_arm.c

While bug 1477929 fixed the obvious build failure, it still allowed the
compiler to break things when it inlines the mpi_arm.c functions into
its callers via LTO.

The problem is that all those assembly blocks take a length as input in
a register, and decrement that register. They also update both registers
they're passed in with pointers, via post-indexed offsets on ldr and str.

But the constraints are not explicit about those writes to the
registers, so the compiler may decide to reuse them as if they had their
original value in code following the inlined code. It actually happily
does so, which leads to interesting crashes.

Comment 4

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8995466 [details] [diff] [review]
Add r/w constraints to modified registers to asm blocks in mpi_arm.c

Review of attachment 8995466 [details] [diff] [review]:
-----------------------------------------------------------------

It would be great to have a little more context in the patch here. Can you put this on phabricator?

Comment 5

[Mike Hommey [:glandium]]

How would phabricator make a difference here? The patch has all the same information I'd put there.

Comment 6

[Mike Hommey [:glandium]]

Oh, more patch context, as in -U8. I don't have arcanist, so phabricator wouldn't help there anyways. I can update the patch to have more context lines, though.

Comment 7

[Mike Hommey [:glandium]]

Attachment: Add r/w constraints to modified registers to asm blocks in mpi_arm.c

Comment 8

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8995897 [details] [diff] [review]
Add r/w constraints to modified registers to asm blocks in mpi_arm.c

Review of attachment 8995897 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me (with my limited ARM assembly foo). Thanks!

Comment 9

[Mike Hommey [:glandium]]

https://hg.mozilla.org/projects/nss/rev/09ab3310e71028dbdbf6846289cb4d3cea249534