Repository navigation
fix(manifest): validate enum fields when parsing manifests - #1034
Conversation
Manifest and manifest list readers cast raw integers from the file straight into ManifestStatus, DataFile::Content and ManifestContent. Out-of-range values then reached switches ending in unreachable(), which is undefined behavior. Validate these fields through the *FromInt helpers at parse time, and return an error instead of unreachable() for an unknown status in ManifestWriter::WriteEntry.
6954c09 to
559066f
Compare
| field_name, required)); | ||
| ICEBERG_RETURN_UNEXPECTED( | ||
| ParseEnumField(DataFileContentFromInt, field_view, manifest_entries, | ||
| proj_data_file(&DataFile::content), field_name, required)); |
There was a problem hiding this comment.
The file_format case just below has no else-if-required branch, so a null in that required field silently keeps the default instead of returning MakeNullError the way every other field in this switch does.
There was a problem hiding this comment.
Good catch, thanks. Fixed in ea36a9c: the kFileFormatFieldId loop now checks required and returns MakeNullError for a null value, same as the other fields in this switch. Added a regression test (RejectsNullRequiredFileFormat) that writes a null file_format via a relaxed write-side schema (bypassing the writer's own non-null enforcement) and asserts the reader rejects it.
ParseDataFile's hand-rolled loop for DataFile::kFileFormatFieldId skipped the field when null instead of checking `required` and returning MakeNullError like every other field in the switch, addressing review feedback on PR apache#1034. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ParseDataFile's hand-rolled loop for DataFile::kFileFormatFieldId skipped the field when null instead of checking `required` and returning MakeNullError like every other field in the switch, addressing review feedback on PR apache#1034.
ea36a9c to
ad1bc98
Compare
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
Thanks @gty404 for fixing this and @shangxinli for the review!
3 participants
Summary
ManifestReaderandManifestListReadercast raw integers from the file straight intoManifestStatus,DataFile::ContentandManifestContent. An out-of-range value in a manifest then reached switches that end inunreachable(), which is undefined behavior.ManifestStatusFromInt,DataFileContentFromIntand a newManifestContentFromInt, returningInvalidArgumentfor unknown values.ManifestWriter::WriteEntrynow returns an error instead ofstd::unreachable()for an unknown entry status.Test plan
data_file.content,statusand manifestcontent(bypassingManifestWriter) and assert the readers returnInvalidArgumentctest(all 20 test targets) passes locally