3 ms·

You are right to be concerned. The primary culprit is the use of C99 variable-length arrays (or alloca when those are not available). I can see why ASAN would w

by derf_ 6y ago

You are right to be concerned. The primary culprit is the use of C99 variable-length arrays (or alloca when those are not available). I can see why ASAN would want to pad those. Fortunately, the allocation sizes of these arrays are tightly constrained by the limits of the Opus format (this is not an accident: I helped design the format). I performed a careful analysis (by hand) to ensure the maximum possible stack usage (without ASAN) was not anywhere close to the size allocated for the stack. It is of course possible that I made mistakes, but I tried very hard not to. The broader point is that this is an entire class of memory access error that ASAN cannot properly validate due to its design. Because it changes the system under test in ways that differ from a real deployment, I cannot use it to mechanically validate my hand analysis. It is worth noting that valgrind does not have this problem (because it instruments the original binary, instead of making compile-time changes), and is also used by Firefox in testing.

inetknght 6y ago
Address Sanitizer isn't a tool that solves every problem. But it solves well the problems it's intended. I'm assuming you're still talking about Firefox running Opus [0]? I'm not familiar with it. But I do think that any platform that can run Firefox and wants to listen to audio should have a lot more than 128k lying around. Otherwise your user isn't using the right tool for the job. I understand memory constraints can be tight and it feels like there's 128k of space right there that you can use. And to achieve that you've written it in C and manually calculated the stack space needed. It feels awesome to have that kind of knowledge and experience with opportunity to leverage it! It's great that you've helped design a media format and implement something to process it! But Firefox wastes tons of megabytes just to load, store, and transmit telemetry, preferences, search providers... the list can go on. Do you really want to try to convince me that a 128k slab for audio that the user specifically loaded is too expensive in the face of all of the telemetry loaded by Firefox these days? Why add all the maintenance headache when you can instead allocate a single slab for your own use? If I were to write it and manually optimize then I'd use an allocated slab. And, as a bonus, the slab's ownership can move to any other thread if needed. And it would make it clean in Address Sanitizer. And, even more important, I wouldn't have to worry about what happens if the frames before my entrypoint changes. For example, Firefox's software design could change and adds a couple of wrappers around the manually-sized stack. What happens if I want to compile Firefox on a different platform which doesn't permit a manually-sized thread stack? So it sounds like you've written something that's going to be very difficult to maintain if you were to vanish. And you've written it for a software product where that level of optimization is, perhaps, better done automatically by tools available to you. If you build a standalone media player specifically for Opus then that'd be pretty cool. But Firefox shouldn't have pieces that are difficult to maintain. [0]: https://hacks.mozilla.org/2018/10/introducing-opus-1-3/ https://hacks.mozilla.org/2018/10/introducing-opus-1-3/
vlovich123 6y ago
Configure using larger stacks when building with ASAN since this is a known problem? The sanitizers are useful enough that I’ve found adding small targeted changes to support those modes to be valuable vs the increased risk of not testing what you run.

by derf_ 6y ago

inetknght 6y ago

Address Sanitizer isn't a tool that solves every problem. But it solves well the problems it's intended. I'm assuming you're still talking about Firefox running Opus [0]? I'm not familiar with it. But I do think that any platform that can run Firefox and wants to listen to audio should have a lot more than 128k lying around. Otherwise your user isn't using the right tool for the job. I understand memory constraints can be tight and it feels like there's 128k of space right there that you can use. And to achieve that you've written it in C and manually calculated the stack space needed. It feels awesome to have that kind of knowledge and experience with opportunity to leverage it! It's great that you've helped design a media format and implement something to process it! But Firefox wastes tons of megabytes just to load, store, and transmit telemetry, preferences, search providers... the list can go on. Do you really want to try to convince me that a 128k slab for audio that the user specifically loaded is too expensive in the face of all of the telemetry loaded by Firefox these days? Why add all the maintenance headache when you can instead allocate a single slab for your own use? If I were to write it and manually optimize then I'd use an allocated slab. And, as a bonus, the slab's ownership can move to any other thread if needed. And it would make it clean in Address Sanitizer. And, even more important, I wouldn't have to worry about what happens if the frames before my entrypoint changes. For example, Firefox's software design could change and adds a couple of wrappers around the manually-sized stack. What happens if I want to compile Firefox on a different platform which doesn't permit a manually-sized thread stack? So it sounds like you've written something that's going to be very difficult to maintain if you were to vanish. And you've written it for a software product where that level of optimization is, perhaps, better done automatically by tools available to you. If you build a standalone media player specifically for Opus then that'd be pretty cool. But Firefox shouldn't have pieces that are difficult to maintain. [0]: https://hacks.mozilla.org/2018/10/introducing-opus-1-3/ https://hacks.mozilla.org/2018/10/introducing-opus-1-3/

vlovich123 6y ago

Configure using larger stacks when building with ASAN since this is a known problem? The sanitizers are useful enough that I’ve found adding small targeted changes to support those modes to be valuable vs the increased risk of not testing what you run.