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

Bug 1847098 - Add version number of so files #103

Merged
merged 2 commits into from
Feb 23, 2024

Conversation

lissyx
Copy link
Contributor

@lissyx lissyx commented Feb 19, 2024

See the Firefox bug for more in depth details.

@lissyx lissyx force-pushed the extract_so_version branch 5 times, most recently from 4dd5e76 to 66010e0 Compare February 23, 2024 15:00
@lissyx lissyx marked this pull request as ready for review February 23, 2024 15:15
The original code would have paniced on non-utf8 paths, and would fail to return any version components if a single one failed to be parsed as a number.

The new code will now gracefully fail if the filename (the rest of the path is inconsequential) is non-utf8, and now tries to handle as many version components as it can, including partial parsing of mixed alphanumeric components such as, eg 2rc5 (which was aready a test case) would be parsed as `25`
@Jake-Shadle Jake-Shadle merged commit c81300e into rust-minidump:main Feb 23, 2024
15 checks passed
@lissyx
Copy link
Contributor Author

lissyx commented Feb 23, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

lissyx commented Feb 24, 2024

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 not 3.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
Copy link
Contributor Author

lissyx commented Feb 24, 2024

(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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants