Segfault due to changing type layouts (context::Input and common::Context)

projectgus commented 1 year ago

I came back to a project that I hadn't worked on for a while and found that it now segfaults calling stream.index() on input!

I can reproduce it here with cargo run --example transcode-x264 video2.mp4 temp.mp4

(video2.mp4 is from samples.ffmpeg.org but I think it'll crash with anything.)

Example output and gdb strack trace

``` x264 options: {"preset": "medium"} [mpeg4 @ 0x555555633580] looks like this file was encoded with (divx4/(old)xvid/opendivx) -> forcing low_delay flag Input #0, mov,mp4,m4a,3gp,3g2,mj2, from '/home/gus/Downloads/video2.mp4': Metadata: major_brand : isom minor_version : 0 compatible_brands: mp41 creation_time : 2008-12-22T23:31:11.000000Z Duration: 00:02:47.23, start: 0.000000, bitrate: 1149 kb/s Stream #0:0[0x1](eng): Video: mpeg4 (mp4v / 0x7634706D), yuv420p, 320x240 [SAR 1:1 DAR 4:3], 1011 kb/s, 29.97 fps, 29.97 tbr, 90k tbn (default) Metadata: creation_time : 2008-12-22T23:31:11.000000Z vendor_id : [0][0][0][0] Stream #0:1[0x2](eng): Data: none (rtp / 0x20707472), 23 kb/s Metadata: creation_time : 2008-12-22T23:31:18.000000Z Stream #0:2[0x5](eng): Audio: aac (LC) (mp4a / 0x6134706D), 44100 Hz, stereo, fltp, 97 kb/s (default) Metadata: creation_time : 2008-12-22T23:31:19.000000Z vendor_id : [0][0][0][0] Stream #0:3[0x6](eng): Data: none (rtp / 0x20707472), 13 kb/s Metadata: creation_time : 2008-12-22T23:31:20.000000Z Stream #0:4[0x7](eng): Data: none (mp4s / 0x7334706D), 0 kb/s (default) Metadata: creation_time : 2008-12-22T23:31:20.000000Z Stream #0:5[0x8](eng): Data: none (mp4s / 0x7334706D) (default) Metadata: creation_time : 2008-12-22T23:31:20.000000Z [libx264 @ 0x5555556b4d80] using SAR=1/1 [libx264 @ 0x5555556b4d80] using cpu capabilities: MMX2 SSE2Fast SSSE3 SSE4.2 AVX FMA3 BMI2 AVX2 [New Thread 0x7fffe4d396c0 (LWP 720894)] [New Thread 0x7fffe45386c0 (LWP 720895)] [New Thread 0x7fffe3d376c0 (LWP 720896)] [New Thread 0x7fffe35366c0 (LWP 720897)] [New Thread 0x7fffe2d356c0 (LWP 720898)] [New Thread 0x7fffe25346c0 (LWP 720899)] [New Thread 0x7fffe1d336c0 (LWP 720900)] [New Thread 0x7fffe0a656c0 (LWP 720901)] [libx264 @ 0x5555556b4d80] profile High, level 1.3, 4:2:0, 8-bit [libx264 @ 0x5555556b4d80] 264 - core 164 r3095 baee400 - H.264/MPEG-4 AVC codec - Copyleft 2003-2022 - http://www.videolan.org/x264.html - options: cabac=1 ref=3 deblock=1:0:0 analyse=0x3:0x113 me=hex subme=7 psy=1 psy_rd=1.00:0.00 mixed_ref=1 me_range=16 chroma_me=1 trellis=1 8x8dct=1 cqm=0 deadzone=21,11 fast_pskip=1 chroma_qp_offset=-2 threads=7 lookahead_threads=1 sliced_threads=0 nr=0 decimate=1 interlaced=0 bluray_compat=0 constrained_intra=0 bframes=3 b_pyramid=2 b_adapt=1 b_bias=0 direct=1 weightb=1 open_gop=1 weightp=2 keyint=250 keyint_min=25 scenecut=40 intra_refresh=0 rc_lookahead=40 rc=crf mbtree=1 crf=23.0 qcomp=0.60 qpmin=0 qpmax=69 qpstep=4 ip_ratio=1.40 aq=1:1.00 Output #0, mp4, to 'test.mp4': Metadata: major_brand : isom minor_version : 0 compatible_brands: mp41 creation_time : 2008-12-22T23:31:11.000000Z Stream #0:0: Video: h264, yuv420p, 320x240 [SAR 1:1 DAR 4:3], q=2-31 Stream #0:1: Audio: aac (LC), 44100 Hz, stereo, fltp, 97 kb/s Thread 1 "transcode-x264" received signal SIGSEGV, Segmentation fault. 0x0000555555573e57 in ffmpeg::format::stream::stream::Stream::as_ptr (self=0x7fffffffc5d0) at src/format/stream/stream.rs:25 25 *(*self.context.as_ptr()).streams.add(self.index) (gdb) bt #0 0x0000555555573e57 in ffmpeg::format::stream::stream::Stream::as_ptr (self=0x7fffffffc5d0) at src/format/stream/stream.rs:25 #1 0x0000555555573f8f in ffmpeg::format::stream::stream::Stream::index (self=0x7fffffffc5d0) at src/format/stream/stream.rs:53 #2 0x0000555555565339 in transcode_x264::main () at examples/transcode-x264.rs:212 #3 0x00005555555704bb in core::ops::function::FnOnce::call_once () at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/core/src/ops/function.rs:250 #4 0x000055555556c69e in std::sys_common::backtrace::__rust_begin_short_backtrace (f=0x555555564460 ) at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/sys_common/backtrace.rs:135 #5 0x0000555555570eb1 in std::rt::lang_start::{closure#0}<()> () at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/rt.rs:166 #6 0x000055555559dd6b in core::ops::function::impls::{impl#2}::call_once<(), (dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe)> () at library/core/src/ops/function.rs:284 #7 std::panicking::try::do_call<&(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe), i32> () at library/std/src/panicking.rs:500 #8 std::panicking::try + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe)> () at library/std/src/panicking.rs:464 #9 std::panic::catch_unwind<&(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe), i32> () at library/std/src/panic.rs:142 #10 std::rt::lang_start_internal::{closure#2} () at library/std/src/rt.rs:148 #11 std::panicking::try::do_call () at library/std/src/panicking.rs:500 #12 std::panicking::try () at library/std/src/panicking.rs:464 #13 std::panic::catch_unwind () at library/std/src/panic.rs:142 #14 std::rt::lang_start_internal () at library/std/src/rt.rs:148 #15 0x0000555555570e8a in std::rt::lang_start<()> (main=0x555555564460 , argc=3, argv=0x7fffffffcad8, sigpipe=0) at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/rt.rs:165 #16 0x0000555555565c6e in main () #17 0x00007ffff5827cd0 in __libc_start_call_main (main=main@entry=0x555555565c50

, argc=argc@entry=3, argv=argv@entry=0x7fffffffcad8) at ../sysdeps/nptl/libc_start_call_main.h:58 #18 0x00007ffff5827d8a in __libc_start_main_impl (main=0x555555565c50

, argc=3, argv=0x7fffffffcad8, init=, fini=, rtld_fini=, stack_end=0x7fffffffcac8) at ../csu/libc-start.c:360 #19 0x0000555555560675 in _start () (gdb) ```

I'm pretty certain the root cause is the transmute_copy call in PacketIter::next() at src/format/context/input.rs:174:

impl<'a> Iterator for PacketIter<'a> {
    type Item = Result<(Stream<'a>, Packet), Error>;

    fn next(&mut self) -> Option<<Self as Iterator>::Item> {
        let mut packet = Packet::empty();

        match packet.read(self.context) {
            Err(Error::Eof) => None,

            Ok(..) => unsafe {
                Some(Ok((
                    Stream::wrap(mem::transmute_copy(&self.context), packet.stream()),
                    packet,
                )))
            },

            Err(err) => Some(Err(err)),
        }
    }
}

The transmute_copy takes a reference to ffmpeg::format::context::Input and returns a ffmpeg::format::context::common::Context.

I think, probably due to a compiler update, the layout of these structs has changed and the ptr field of Input (ptr: *mut AVFormatContext) is no longer at the same offset as the ptr in Context.

❯ cargo rustc -- -V
[...]
rustc 1.72.0 (5680fa18f 2023-08-23)

❯ cargo clean
❯ RUSTC_BOOTSTRAP=1 cargo rustc -- -Zprint-type-sizes
[...]
print-type-size type: `format::context::common::Context`: 16 bytes, alignment: 8 bytes
print-type-size     field `.dtor`: 8 bytes
print-type-size     field `.ptr`: 8 bytes
[...]
print-type-size type: `format::context::input::Input`: 48 bytes, alignment: 8 bytes
print-type-size     field `._io`: 24 bytes
print-type-size     field `.ctx`: 16 bytes
print-type-size     field `.ptr`: 8 bytes
[...]

Marking both structs with repr(C) seems to fix this, but doesn't feel like the right fix. Can submit that as a stop-gap PR if you like, though?
I don't fully understand transmuting "Input" to become "Context". From what I can tell, the goal is that the Stream in the return value should have a context that wraps the Input.ptr AVFormatContext pointer at self.context.ptr (not the one at self.context.ctx.ptr). I played around with adding a field to PacketIter so it holds a single Stream or a packet_context that can then be referenced in each iterator result, but the result of next() has a different lifetime to the iterator and this feels like the wrong approach.

EDIT: Never mind, I missed these ptrs are actually the same. Will submit a PR.

I don't know much about libavcontext, and pretty new to Rust, so this is about the point I ran out of ideas. :). However I'm very happy to provide more information or try to cobble together a fix if you have any suggestions.

Thanks for maintaining this library, it's a big improvement on raw FFI to libavcontext (or writing the same code in C.) :grin:

projectgus commented 1 year ago

I realised while fixing this that Input implements Deref to Context, so it's reasonable in most places to expect it to do the right thing.

I think because transmute_copy allows unrelated argument types then this probably wasn't happening, though, and the compiler is doing the simplest thing (take in Input, copy it and call it Context). And it just happened to work most of the time, as the ptr fields were both first in the memory layout...

projectgus commented 1 year ago

Just to satisfy my own uncertainty, seems the type layout changed between rustc v1.69 and v1.72:

❯ RUSTC_BOOTSTRAP=1 cargo +1.69 rustc -- -Zprint-type-sizes
[...]
print-type-size type: `format::context::input::Input`: 48 bytes, alignment: 8 bytes
print-type-size     field `.ctx`: 16 bytes
print-type-size     field `.ptr`: 8 bytes
print-type-size     field `._io`: 24 bytes
[...]
print-type-size type: `format::context::common::Context`: 16 bytes, alignment: 8 bytes
print-type-size     field `.ptr`: 8 bytes
print-type-size     field `.dtor`: 8 bytes

tilpner commented 1 year ago

Thank you! That's a good analysis, I think you're right about every step of why this stopped "working". I'll address the fixes in your PR over there. :)

meh / rust-ffmpeg

Segfault due to changing type layouts (context::Input and common::Context) #176