bitstream: fixes #257
bitstream: fixes #257
Conversation
|
More issues that I have:
|
| @@ -91,6 +91,9 @@ class BitReader | |||
| ret = cache >> (32-n); | |||
| //Cache next | |||
| Cache(); | |||
| //Check available | |||
| if (cached<a) | |||
There was a problem hiding this comment.
shouldn't this check be done inside GetChached() ?
There was a problem hiding this comment.
GetCached() is called from two places, and only this call site needs the check. but if you prefer to move it there for clarity, we can do that too
There was a problem hiding this comment.
better do it in the inner most layer always
| @@ -414,6 +422,8 @@ class BitWritter{ | |||
| //Increase cached | |||
| cached = b; | |||
| } else { | |||
| //Don't invoke UB if empty | |||
| if (!n) return v; | |||
There was a problem hiding this comment.
should we put this check on the else?
There was a problem hiding this comment.
the idea was to skip the FlushCache() call afterwards, but yes, we can also do that if you prefer
There was a problem hiding this comment.
we can put it at the begginging of the function, or as a first clause of the if-else-if
| @@ -414,6 +422,8 @@ class BitWritter{ | |||
| //Increase cached | |||
| cached = b; | |||
| } else { | |||
There was a problem hiding this comment.
| } else { | |
| } else if (n) { |
Fixes several issues in bitstream.h (BitReader and BitWriter):
in C++, it is undefined behavior to do
dword >> 32ordword << 32. And we are doing that in several points of the code. This is one of the UBs that hardly leads to crashes, but it did lead to an actual bug, which is observed in h264: full SPS parsing #253 (see description). InGet(), when we combine cache with ret:media-server/include/bitstream.h
Line 91 in 6b369a1
This assumes that the lower bits of
cacheare zero. Which is guaranteed sinceSkipCached()does a right shift:media-server/include/bitstream.h
Line 268 in 6b369a1
...except when
nis 32, in which case this invokes UB. In most implementations / architectures this will result in leavingcacheunchanged rather than zeroed.GetVariableLengthUnsignedmakes no sense. It is an AV1 primitive (essentially identical to H.264ue(v)) and it should be implemented as:Get()doesn't set error condition if there are not enough bits to read (or rather, it only does that in specific cases).