Repository navigation
core: treat malformed definition/data messages as corruption, not crashes - #128
Conversation
1199655 to
1b83cfa
Compare
…shes ULog() raised uncaught exceptions on malformed .ulg files: KeyError (_parse_nested_type, an ADD_LOGGED_MSG referencing an undefined/nested format), ValueError (_extract_type array spec), struct.error (the definitions phase only caught IndexError), and OverflowError (file.seek with a bogus appended offset from FLAG_BITS). Any service parsing an untrusted log was taken down unless every call was wrapped in try/except. - _read_file_definitions: move the header parse into the try and broaden the handler to (IndexError, ValueError, KeyError, OverflowError, struct.error), treating a malformed definition message as corruption like _read_file_data already does. The intentional "unknown incompatible flag" aborts (ValueError / NotImplementedError) are preserved and still propagate. - _read_file_data: broaden the per-message handler to also catch KeyError, ValueError and OverflowError. - _extract_type: reject an out-of-range array size (also prevents a range()-based resource-exhaustion DoS). - _load_file: guard the appended-data seek against a bogus offset. Fuzzing ULog() on mutated valid logs (seed=1, 4000 iterations) went from 10 distinct escaping exception sites to 3 - and those 3 are the library's intended invalid-header / incompatible-flag refusals. Fixes PX4#127
1b83cfa to
a6b45a4
Compare
bkueng
left a comment
There was a problem hiding this comment.
Flight review already has a broad exception capture around parsing.
This makes sense nonetheless though.
And I would directly add a dedicated ULogException while you're at it (with a separate class for the incompatible flag case).
| except (IndexError, ValueError, KeyError, OverflowError, struct.error) as exc: | ||
| # Keep the intentional incompatible-flag abort ("cannot parse the | ||
| # log"); treat any other failure as corruption, like _read_file_data. | ||
| if isinstance(exc, ValueError) and 'cannot parse the log' in str(exc): |
There was a problem hiding this comment.
Can you add a dedicated exception so no string check is required?
There was a problem hiding this comment.
Done in 0d297a9: the incompatible-flag case now raises ULogIncompatibleFlagException (a ULogException subclass), which the corruption handler doesn't catch, so no string check is needed.
Use dedicated exceptions instead of matching the error message to tell the intentional incompatible-flag abort apart from file corruption. - Add pyulog/exceptions.py with ULogException (the file cannot be parsed) and its subclass ULogIncompatibleFlagException, both exported from pyulog. They live in their own module as core.py is right at pylint's max-module-lines. - Raise ULogException for an invalid file header (was TypeError) and ULogIncompatibleFlagException for an unknown incompatible flag (was ValueError / NotImplementedError). They are not caught by the corruption handlers, so the string check is gone. - Let UnicodeDecodeError propagate again: it is a ValueError subclass, so the broadened handlers turned string errors into corruption even with disable_str_exceptions=False. - Only guard the appended-data seek(), not _read_file_data(). - Add tests for both exceptions, corrupt definitions and strict string parsing.
|
Thanks for the review! Good to know Flight Review already catches this, so the PR is mainly about robustness for other users of the library. Updated in 0d297a9:
Note that this changes the exception types for these cases, so a caller catching |
bkueng
left a comment
There was a problem hiding this comment.
Nice, thanks!
Note that this changes the exception types for these cases, so a caller catching TypeError / ValueError there would need ULogException instead. If you prefer, I can also derive the new classes from the old built-in types for backward compatibility.
I'll mention it in the release notes, that should be sufficient
bkueng
left a comment
There was a problem hiding this comment.
Nice, thanks!
Note that this changes the exception types for these cases, so a caller catching TypeError / ValueError there would need ULogException instead. If you prefer, I can also derive the new classes from the old built-in types for backward compatibility.
I'll mention it in the release notes, that should be sufficient
What
ULog()raises uncaught exceptions on malformed.ulginput, aborting the parse (reported in #127). The definitions phase only guardedIndexError, and the data phase only guardedIndexError/ end-of-filestruct.error, so several failure modes escaped:KeyError(dominant)_parse_nested_typeADD_LOGGED_MSGreferencing an undefined / nested formatValueError_extract_typeint(...)); also an unclampedarray_size→range()DoSstruct.error_MessageInfo/_MessageFlagBits/_MessageHeaderstruct.unpackin the definitions phase (onlyIndexErrorwas caught there)OverflowError_load_filefile.seek()with a bogus appended offset fromFLAG_BITSFix
Treat these as corruption, consistent with the existing
IndexErrorhandling:_read_file_definitions: move the header parse into thetryand broaden the handler to(IndexError, ValueError, KeyError, OverflowError, struct.error)._read_file_data: broaden the per-message handler to also catchKeyError,ValueError,OverflowError._extract_type: reject an out-of-rangearray_size(also prevents arange()-based resource-exhaustion DoS)._load_file: guard the appended-dataseek()against a bogus offset.String decoding errors with
disable_str_exceptions=False(UnicodeDecodeError) still propagate, as before.Valid logs are unaffected; only malformed records are skipped / flagged (
file_corruptionstaysTrue).Dedicated exceptions
Intentional refusals now use dedicated exceptions (new
pyulog/exceptions.py, exported frompyulog) instead of generic built-ins:TypeErrorULogExceptionValueError/NotImplementedErrorULogIncompatibleFlagException(subclass ofULogException)Callers can catch every intentional refusal with a single
except ULogException.Verification
Fuzzing
ULog()on mutated valid logs (seed=1, 4000 iterations):The 3 that remain are the intended refusals above (
ULogExceptionfor an invalid header,ULogIncompatibleFlagExceptionat the two incompatible-flag checks). The genuine robustness gaps (KeyError,_extract_typeValueError, definitions-phasestruct.error,_load_fileOverflowError) are gone.New tests in
test/test_ulog.pycover both exceptions, corrupt definitions and strict string parsing;pytestandpylintpass.Fixes #127