dcuddeback / libusb-rs

A safe Rust wrapper for libusb.
MIT License
199 stars 64 forks source link

Fix use-after-free in tests, and mark corresponding functions unsafe #10

Closed kevinmehall closed 7 years ago

kevinmehall commented 7 years ago

The from_libusb functions turn a raw pointer into a struct with safe methods and destructors that dereference the pointer. These functions are currently misused by the tests to violate memory safety in "safe" code -- the test_helpers macros return structures containing pointers to memory that was freed when the Vec goes out of scope within the macro.

Since there isn't a good way to move the scope of the Vec out to the function level besides manually expanding the macros, I made the macros simply leak the Vec. It may be better to reconsider the use of macros here, and construct the test data statically or in the scope of the test function.

The consequences of the use-after-free are masked by jemalloc, but show up as segfaults and in valgrind when compiled with `alloc_system:

==26815== Thread 2 interface_descriptor::test::it_has_sub_class_code:
==26815== Invalid read of size 1
==26815==    at 0x10001396F: libusb::interface_descriptor::InterfaceDescriptor::sub_class_code::h38a592abd132663a (interface_descriptor.rs:72)
==26815==    by 0x1000417F3: libusb::interface_descriptor::test::it_has_sub_class_code::_$u7b$$u7b$closure$u7d$$u7d$::hc5ed194dfd7e5555 (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x1000051E4: core::ops::impls::_$LT$impl$u20$core..ops..FnOnce$LT$A$GT$$u20$for$u20$$RF$$u27$a$u20$mut$u20$F$GT$::call_once::hbda3c0beb97d3534 (ops.rs:1988)
==26815==    by 0x100001F05: _$LT$core..option..Option$LT$T$GT$$GT$::map::h93c910a9c81c2638 (option.rs:385)
==26815==    by 0x1000082F0: _$LT$core..iter..Map$LT$I$C$$u20$F$GT$$u20$as$u20$core..iter..iterator..Iterator$GT$::next::h8524024f68dc0877 (mod.rs:899)
==26815==    by 0x100009423: _$LT$collections..vec..Vec$LT$T$GT$$u20$as$u20$core..iter..traits..FromIterator$LT$T$GT$$GT$::from_iter::h2c5c62bed8233cb2 (vec.rs:1346)
==26815==    by 0x100005769: core::iter::iterator::Iterator::collect::h2bc38768bba520ee (iterator.rs:1207)
==26815==    by 0x100015030: libusb::interface_descriptor::test::it_has_sub_class_code::hf6bf1e8d2b53791d (<std macros>:177)
==26815==    by 0x10004BACB: _$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$::call_box::h6c46b1f51a5f97cd (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x10006043A: test::run_test::run_test_inner::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::h264d6ff770d7074c (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x1000437AD: std::panicking::try::call::h8d6a00e6c0ac8b9e (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x1000854DB: __rust_try (in target/debug/libusb-8225a624035221f7)
==26815==  Address 0x1012c4fb6 is 6 bytes inside a block of size 40 free'd
==26815==    at 0x10014D2F7: free (in /usr/local/Cellar/valgrind/3.11.0/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==26815==    by 0x100006FB4: alloc::heap::deallocate::hdab899090772cab5 (heap.rs:113)
==26815==    by 0x100007682: _$LT$alloc..raw_vec..RawVec$LT$T$GT$$u20$as$u20$core..ops..Drop$GT$::drop::h36136a8b377fd1bd (raw_vec.rs:568)
==26815==    by 0x100005EF0: drop::ha3dffa6aae84313e (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x100001958: drop_contents::hb4e29eef57f894ba (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x100005F4B: drop::hb4e29eef57f894ba (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x100014FC8: libusb::interface_descriptor::test::it_has_sub_class_code::hf6bf1e8d2b53791d (test_helpers.rs:86)
==26815==    by 0x10004BACB: _$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$::call_box::h6c46b1f51a5f97cd (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x10006043A: test::run_test::run_test_inner::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::h264d6ff770d7074c (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x1000437AD: std::panicking::try::call::h8d6a00e6c0ac8b9e (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x1000854DB: __rust_try (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x100085395: __rust_maybe_catch_panic (in target/debug/libusb-8225a624035221f7)
==26815==  Block was alloc'd at
==26815==    at 0x10014CEBB: malloc (in /usr/local/Cellar/valgrind/3.11.0/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==26815==    by 0x1000070CC: alloc::heap::allocate::h252d38a38e7da6a5 (heap.rs:59)
==26815==    by 0x100007070: alloc::heap::exchange_malloc::hf72cbcfe51f02d6d (heap.rs:137)
==26815==    by 0x100014DF1: libusb::interface_descriptor::test::it_has_sub_class_code::hf6bf1e8d2b53791d (<std macros>:3)
==26815==    by 0x10004BACB: _$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$::call_box::h6c46b1f51a5f97cd (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x10006043A: test::run_test::run_test_inner::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::h264d6ff770d7074c (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x1000437AD: std::panicking::try::call::h8d6a00e6c0ac8b9e (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x1000854DB: __rust_try (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x100085395: __rust_maybe_catch_panic (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x10005E185: std::thread::Builder::spawn::_$u7b$$u7b$closure$u7d$$u7d$::h9e5d89388555e457 (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x10004B878: _$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$::call_box::h173484225f9723d2 (in target/debug/libusb-8225a624035221f7)
==26815==    by 0x1000832A8: std::sys::thread::Thread::new::thread_start::h8f3bd45211e9f5ea (in target/debug/libusb-8225a624035221f7)
==26815== 
dcuddeback commented 7 years ago

@kevinmehall Good catch. I like the idea of reconsidering the use of macros. I'll merge this, because leaking is an improvement over use-after-free, and then I'll look into a more permanent fix.

dcuddeback commented 7 years ago

Merged on the command-line, because the changes from #9 required an additional unsafe block.