Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong code generated for Android ARM with optimisations #22776

Closed
romanb opened this issue Feb 24, 2015 · 4 comments
Closed

Wrong code generated for Android ARM with optimisations #22776

romanb opened this issue Feb 24, 2015 · 4 comments
Labels
A-codegen Area: Code generation

Comments

@romanb
Copy link

romanb commented Feb 24, 2015

There appears to be a case that looks like optimisations resulting in bad code to be generated for Android ARM. This issue demonstrates the problem in the context where I ran into it, using the byteorder library.

Test environment

  • Host OS: OSX 10.9.5
  • Android SDK
  • Android NDK (Revision 10d) with standalone toolchain according to the wiki.
  • Android Emulator: ARM (armeabi-v7a), Android 5.0.1 (API level 21)
  • rustc 1.0.0-dev (built 2015-02-23), configured for cross-compilation to the arm-linux-androideabi target.

Steps to reproduce

  1. Clone byteorder. Checkout commit d9259c15a4b18de37d765aebbe5034f282772b84.

  2. Create main.rs:

    use byteorder::{BigEndian, WriteBytesExt};
    
    extern crate byteorder;
    
    fn main() {
        let mut v_16: Vec<u8> = Vec::new();
        v_16.write_u16::<BigEndian>(64_u16).unwrap();
        println!("u16_be: {:?}", v_16);
    
        let mut v_32: Vec<u8> = Vec::new();
        v_32.write_u32::<BigEndian>(64_u32).unwrap();
        println!("u32_be: {:?}", v_32);
    
        let mut v_64: Vec<u8> = Vec::new();
        v_64.write_u64::<BigEndian>(64_u64).unwrap();
        println!("u64_be: {:?}", v_64);
    }
    
  3. Build

    mkdir target
    # Build byteorder
    rustc -C opt-level=3 --target=arm-linux-androideabi -C linker=arm-linux-androideabi-gcc -C ar=arm-linux-androideabi-ar --crate-type lib --out-dir target/ --emit=dep-info,link /path/to/byteorder/src/lib.rs
    # Build main.rs
    rustc -C opt-level=3 --target=arm-linux-androideabi -C linker=arm-linux-androideabi-gcc -C link-args=-pie --crate-type bin -Ltarget --out-dir target/ ./main.rs
    
  4. Run on emulator

    adb push target/main /some/writeable/dir/main
    adb shell /some/writeable/dir/main
    
  5. Output

    Expected:

    u16_be: [0, 64]
    u32_be: [0, 0, 0, 64]
    u64_be: [0, 0, 0, 0, 0, 0, 0, 64]
    

    Actual (sample):

    u16_be: [253, 182]
    u32_be: [57, 0, 0, 0]
    u64_be: [244, 102, 251, 255, 57, 0, 0, 0]
    

Workarounds

There seem to be two ways to work around the problem:

  1. Apply the following patch to byteorder

    diff --git a/src/lib.rs b/src/lib.rs
    index 009a780..8e6f37d 100644
    --- a/src/lib.rs
    +++ b/src/lib.rs
    @@ -265,8 +265,8 @@ macro_rules! write_num_bytes {
    
             assert!($dst.len() >= $size); // critical for memory safety!
             unsafe {
    -            let bytes = (&transmute::<_, [u8; $size]>($n.$which())).as_ptr();
    -            copy_nonoverlapping_memory($dst.as_mut_ptr(), bytes, $size);
    +            let bytes = transmute::<_, [u8; $size]>($n.$which());
    +            copy_nonoverlapping_memory($dst.as_mut_ptr(), (&bytes).as_ptr(), $size);
             }
         });
     }
    
  2. Turn off optimizations (opt-level).

Further Details

The following is the output for the relevant functions (write_u16, write_u32 and write_u64) of compiling byteorder using --emit=llvm-ir with and without the above patch.

Without the patch: https://gist.github.com/romanb/2ff2ebb474c87103e631
With the patch: https://gist.github.com/romanb/d419ba4882d9c489c494

@kmcallister kmcallister changed the title Bad code generated for Android ARM with optimisations Wrong code generated for Android ARM with optimisations Feb 25, 2015
@kmcallister kmcallister added I-wrong A-codegen Area: Code generation labels Feb 25, 2015
romanb pushed a commit to romanb/byteorder that referenced this issue Feb 25, 2015
@kennytm
Copy link
Member

kennytm commented Mar 14, 2015

Not reproducible.

  • Host: OS X 10.10
  • NDK: r10d
  • Emulator: Android 4.4.2 with ARM
  • rustc is built using the nightly source from 2015-03-11.

@romanb
Copy link
Author

romanb commented Mar 15, 2015

I tried the latest nightly on a different host OS, unfortunately still reproducible:

  • Host: Ubuntu 14.10
  • NDK: r10d
  • Emulator: Android 5.0.1 (armeabi-v7a)
  • rustc nightly: rustc 1.0.0-dev (built 2015-03-15)

@romanb
Copy link
Author

romanb commented Apr 25, 2015

Just another update: Reproduced with Rust Beta2 and a newer version of byteorder (which accidentally but thankfully reintroduced the issue but is now patched again, so there is a recent commit that compiles with the latest rustc). Specifically:

@huonw
Copy link
Member

huonw commented May 2, 2016

I think the problem here is (the old) byteorder: .as_ptr only returns a pointer that is valid for as long as the slice it is called on, and the slice in the original code only lasts until the end of its let statement. Thus, the original code actually had a use-after-free bug.

I'm closing since I don't think this is a bug in Rust, and thanks to @romanb for diagnosing and even fixing the problem in byteorder!

@huonw huonw closed this as completed May 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation
Projects
None yet
Development

No branches or pull requests

4 participants