Find extensions whose version directory has a single component - #346
Find extensions whose version directory has a single component#346dchaudhari7177 wants to merge 1 commit into
Conversation
`load_extension_manifest` globbed version directories with "*.*_*", which
requires a dot. An extension unpacked to `7_0` was never listed, so it was
reported unreadable -- `Error opening manifest info for extension ...`, no name,
no version, no permissions -- with its manifest.json sitting there intact. The
trailing `_<n>` unpack counter is the real marker, so the glob widens to "*_*"
and the dot's other job, keeping out extraneous files like $I30 from FTK, moves
to an explicit is_dir() filter.
The sort key had a second, separate bug. It split the directory name on '.'
without stripping the `_<n>` counter first, so the last component of every
ordinary name was '3_0' rather than '3'. That is not a digit, so every name fell
to the non-numeric branch there and its last component was compared as a string:
1.2.3_0 sorted above 1.2.10_0 because '3_0' > '10_0'. The counter is now
stripped before the split, which fixes ordering for all names, not just the
single-component ones the glob change admits.
The issue reports this second bug as `int('7_0') == 70` accepting underscores in
an integer literal. That path is already gone -- the isdigit() guard added for
non-numeric components sends '7_0' to the string branch rather than int() -- but
the string branch is itself wrong for the same reason, and affects every
extension rather than only oddly-named ones.
Verified on the reproduction the issue names: extension pjkljhegncpnkpknbcohdijeoejaedia
with version directory `7_0`. Three new tests fail on base and pass here; the
full suite is 243 passed, 2 skipped.
Two test cases adopted from dchaudhari7177's independent fix for the same issue in PR #346, which covered ground mine did not. The first is the ordering of single- against multi-component versions. Widening the glob means both shapes can now turn up side by side, so they have to compare against each other and not only among themselves. The second replaces my stray-file test, which used a name that sorted last and so passed with or without the is_dir() filter it was supposed to be pinning. Theirs uses a name that sorts first, which is the case that actually exercises the filter. Reaching the right manifest is not enough on its own even then, since opening a path under a file raises OSError and the loop moves on, so the test also asserts that nothing was logged at error: the right answer by way of a logged error is not the same as the right answer.
|
Thanks for this, and sorry for the collision. I had #345 open for the same issue a few minutes before yours, so I'm going to land that one and close this. Your read on the sort key is right, and it's the same conclusion I reached independently: the Two of your tests were better than mine and I've pulled them into #345, credited in the commit message. The single against multi-component ordering case I didn't have at all. The non-directory test I did have, but mine used a stray name that sorted last, so it passed whether or not the is_dir() filter was there; yours uses one that sorts first, which is the case that actually exercises it. I added an assertion that nothing gets logged at error, since reaching the right manifest by way of a logged error isn't the same as reaching it cleanly. One thing worth knowing for next time: this fix moves the magnet.ctf_2018 corpus baseline (Extensions goes from 3 records to 4, and the status from partial to complete), so |
2 participants
Closes #344.
load_extension_manifestglobbed version directories with"*.*_*", which requires a dot. An extension unpacked to7_0was never listed, so it was reported unreadable with itsmanifest.jsonsitting there intact. The glob widens to"*_*"— the trailing_<n>unpack counter is the real marker — and the dot's other job, keeping out extraneous files like$I30from FTK, moves to an explicitis_dir()filter.On the second bug
The issue reports it as
int('7_0') == 70, since Python accepts underscores in integer literals. That path is already gone: theisdigit()guard added for non-numeric components sends'7_0'to the string branch rather than toint(). But the underlying mistake — not stripping the_<n>counter before splitting on'.'— is still there, and the string branch is wrong for the same reason. It is also much wider than the issue suggests:The last component of every ordinary version directory is
'3_0', not'3'. That is not a digit, so every name falls to the non-numeric branch on its last component and is compared as a string. Measured onmain:main1.2.3_0,1.2.10_01.2.3_01.2.10_07_07_0'3_0' > '10_0'as strings, so the older version wins. The existingtest_highest_numeric_version_still_winsdoes not catch it because its two names differ in the second component, where the numeric branch is still reached.So the counter is now stripped before the split, which fixes ordering for all names rather than only the single-component ones the glob change admits.
Verification
Reproduced on exactly the case the issue names — extension
pjkljhegncpnkpknbcohdijeoejaediawith version directory7_0, the only single-component version directory in the corpus.Four new tests. Three of them fail on
mainand pass here:The fourth pins that a non-directory matching the widened glob is still ignored, which is what the old dot was buying.
Full suite on this branch: 243 passed, 2 skipped, 80 subtests passed. Python 3.12.4, Windows.
One thing left open
The issue also floats filtering on
^\d+(\.\d+)*_\d+$instead of the glob. I went with the glob-plus-is_dir()option because the sort key already ranks malformed names last deliberately (test_a_real_version_is_preferred_over_a_malformed_one), and a regex filter would drop those directories entirely rather than deprioritising them — a behaviour change beyond this issue. Happy to switch if you would rather they were excluded outright.