search

rust-minidump / minidump-writer

Rust rewrite of breakpad's minidump_writer
MIT License
68 stars 17 forks source link

Bug 1847098 - Add version number of so files #103

Closed lissyx closed 8 months ago

lissyx commented 9 months ago

See the Firefox bug for more in depth details.

lissyx commented 8 months ago

Thanks, however

and would fail to return any version components if a single one failed to be parsed as a number.

Please note this was done on purpose

Jake-Shadle commented 8 months ago

Please note this was done on purpose

Can you explain why? In the test data that was available, which I assume was handpicked from real crash reports to represent various different real .so versions encountered in the wild, the only one that failed to be parsed because it wasn't a number, libdbus-1.so.3.34.2rc5. It seemed wasteful to drop all the version components if 1 failed to parse since it's clear that other than the 2rc5 part it's just a regular triplet, and that can be parsed to an integer as well, either partially or fully. If there is a concrete case where this more lenient parsing causes confusion or ambiguity in reports then for sure the logic can be modified to handle that. One thing that can be done in this particular case is parse 2 numbers from the final release/patch component so that there is no ambiguity between .2pre5 and .25.

lissyx commented 8 months ago

Please note this was done on purpose

Can you explain why? In the test data that was available, which I assume was handpicked from real crash reports to represent various different real .so versions encountered in the wild, the only one that failed to be parsed because it wasn't a number, libdbus-1.so.3.34.2rc5. It seemed wasteful to drop all the version components if 1 failed to parse since it's clear that other than the 2rc5 part it's just a regular triplet, and that can be parsed to an integer as well, either partially or fully. If there is a concrete case where this more lenient parsing causes confusion or ambiguity in reports then for sure the logic can be modified to handle that. One thing that can be done in this particular case is parse 2 numbers from the final release/patch component so that there is no ambiguity between .2pre5 and .25.

Well I was not sure it was wise to parse and return something that could be misleading, 3.34.2rc5`` is really not3.34.25and I preferred we got nothing. As mentionned on the bugzilla entry, we wont be able to cover all cases anyway for libs that do not have anything or just explicit 0, and we are going to work on also collecting data during debug symbols productions. Obviously, this might feel disconnected with therust-minidump` project since it means more firefox-specific focus.

Please note that I verified on several distros (ubuntu and debian with apt-file, freebsd and openbsd) and I could not find any library with non digit versions, so in fact the example in the test is made out of my mind (but libabseil example is not). Technically, using strings and not just digits works, but I failed to find any real-life example.

lissyx commented 8 months ago

(and at some point, wether we parse it or just return 0 probably can make sense in both cases, we just need to do the same on rust-minidump and on gecko side)