Fix currentLine out of bounds error #1810
Conversation
|
@zricethezav, @thewizzy noticed this issue in our CI run that included some decoding tests: https://github.com/leaktk/scanner/actions/runs/14210109004/job/39815606323 It thought the matchlen was 128 when len(currentRaw) was only 95. I think this should fix the issue and be a little more robust with future decoding cases. @thewizzy, can you point the failing branch at the fork and confirm the fix as well? I tested it locally when making the change and it resolved it. |
Because `segment.adjustMatchIndex(matchIndex)` was above `segment.lineEndIndex(currentRaw, matchIndex[1]-matchIndex[0])` there were cases where it would cause an out of bounds error. This change resolves the issue by just assuming the whole segment is a opaque chunk of text and starts at the boundaries of the segment instead so that the order doesn't matter as much. The other option would have been to reorder the assignments and add tests to account for that case, but I figured this option is less likely to have gotchas when working with different encoding formats at the potential expense of getting an extra line or two if an encoding format in the future ever allows multiple lines of encoded content to be considered a single segment.
cc42ba7 to
e4cae2f
Compare
I pointed it to the fix and it resolved the issue in our tests. |
|
LGTM, thanks guys 👍🏻 |
Because `segment.adjustMatchIndex(matchIndex)` was above `segment.lineEndIndex(currentRaw, matchIndex[1]-matchIndex[0])` there were cases where it would cause an out of bounds error. This change resolves the issue by just assuming the whole segment is a opaque chunk of text and starts at the boundaries of the segment instead so that the order doesn't matter as much. The other option would have been to reorder the assignments and add tests to account for that case, but I figured this option is less likely to have gotchas when working with different encoding formats at the potential expense of getting an extra line or two if an encoding format in the future ever allows multiple lines of encoded content to be considered a single segment.
Description:
segment.adjustMatchIndex(matchIndex)was abovesegment.lineEndIndex(currentRaw, matchIndex[1]-matchIndex[0])there were cases where it would cause an out of bounds error.This change resolves the issue by just assuming the whole segment is a opaque chunk of text and starts at the boundaries of the segment instead so that the order doesn't matter as much.
The other option would have been to reorder the assignments and add tests to account for that case, but I figured this option is less likely to have gotchas when working with different encoding formats at the potential expense of getting an extra line or two if an encoding format in the future ever allows multiple lines of encoded content to be considered a single segment.
Explain the purpose of the PR.
Checklist: