Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a regression from commit 22106f5d33628515d22c09c1c15dfd2217535116 #1596

Conversation

DemiMarie
Copy link
Contributor

Commit 22106f5 assumed that
dataLength() would always return a nonzero number. Unfortunately, that
isn’t the case: dataLength() returns zero for RPM_NULL_TYPE. This meant
that hdrblobVerifyInfo() failed to reject such entries, which are
invalid.

This fixes the problem in three different ways:

  1. It checks that tag data entries have length greater than zero.
  2. It modifies hdrchkType() to reject RPM_NULL_TYPE.
  3. It modifies dataLength() to consider zero length as an error.

Commit 22106f5 assumed that
dataLength() would always return a nonzero number.  Unfortunately, that
isn’t the case: dataLength() returns zero for RPM_NULL_TYPE.  This meant
that hdrblobVerifyInfo() failed to reject such entries, which are
invalid.

This fixes the problem in three different ways:

1. It checks that tag data entries have length greater than zero.
2. It modifies hdrchkType() to reject RPM_NULL_TYPE.
3. It modifies dataLength() to consider zero length as an error.
It is undefined behavior in C.
@pmatilai
Copy link
Member

Sigh. I'm kinda tempted to just revert the previous commit instead because when you actually look at it:

len comes from dataLenght(), to which we pass the upper limit so it cannot be larger than the header. So it is in range.

I'm really, really fed up with these kind of patches now.

@pmatilai
Copy link
Member

Replaced by #1597. I will not consider any patches in this direction unless there's a concrete reproducable bug involved, far too many regressions by these theoretical hardening patches.

Thanks for pointing out the regression of course.

@pmatilai pmatilai closed this Mar 24, 2021
@DemiMarie DemiMarie deleted the fix-pr-1497-regression branch March 24, 2021 17:30
@DemiMarie
Copy link
Contributor Author

Replaced by #1597. I will not consider any patches in this direction unless there's a concrete reproducable bug involved, far too many regressions by these theoretical hardening patches.

For clarification: does “undefined behavior detected by sanitizers” qualify?

@pmatilai
Copy link
Member

As a rule of thumb: if it shows up in valgrind (or address sanitizer), it's a bug that needs to be addressed.
Obviously there are bugs that will not show up in valgrind that still need fixing - that's just a ballpark rule. As for ubsan, take it with a grain of salt: I'm far from convinced it actually honors the used compiler standard, and so will cause all manner of false alarms to be tripped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants