openpreserve / jhove

File validation and characterisation.
http://jhove.openpreservation.org
Other
171 stars 79 forks source link

Initialize string-valued tokens with empty string #806

Closed prettybits closed 1 year ago

prettybits commented 1 year ago

Related to #668, a null-pointer exception is thrown when adding string properties from docInfo in readDocInfoDict when the token values are an empty hex string. The parser only sets the value of a Literal's underlying StringValuedToken when there are other tokens between < and >, so empty hex strings result in the token's _value being null, while Property expects an actual string.

This either needs to more stringent checking for null at the relevant call sites or just initializing the _value as an empty string for StringValuedToken. AFAICS this should pose no issues, does it? @carlwilson

codecov[bot] commented 1 year ago

Codecov Report

Base: 46.22% // Head: 46.22% // No change to project coverage :thumbsup:

Coverage data is based on head (f0b79d1) compared to base (b54fa00). Patch has no changes to coverable lines.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## integration #806 +/- ## ============================================== Coverage 46.22% 46.22% Complexity 1056 1056 ============================================== Files 57 57 Lines 9057 9057 Branches 1607 1607 ============================================== Hits 4187 4187 Misses 4330 4330 Partials 540 540 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openpreserve). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openpreserve)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

carlwilson commented 1 year ago

This doesn't create any issues and is a belt and braces catch for a fix submitted by @samalloing. I'm going to merge it all the same, the code base is too casual in its treatment of nulls.

prettybits commented 1 year ago

Thanks for merging, I just saw there was also #820 that is effectively concerned with the same issue this PR also fixes. With this merged the added null check there in addStringProperty should actually not even be needed now, but for sure doesn't hurt. In any case, the affected PDFs validate all the same now. :)