bitstream: fixes

[mildsunrise]

Fixes several issues in bitstream.h (BitReader and BitWriter):

• in C++, it is undefined behavior to do dword >> 32 or dword << 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 #253 (see description). In Get(), when we combine cache with ret:

  https://github.com/medooze/media-server/blob/6b369a170791bf25f80b056e6f83855e5a5e6291/include/bitstream.h#L91

  This assumes that the lower bits of cache are zero. Which is guaranteed since SkipCached() does a right shift:

  https://github.com/medooze/media-server/blob/6b369a170791bf25f80b056e6f83855e5a5e6291/include/bitstream.h#L268

  ...except when n is 32, in which case this invokes UB. In most implementations / architectures this will result in leaving cache unchanged rather than zeroed.

• GetVariableLengthUnsigned makes no sense. It is an AV1 primitive (essentially identical to H.264 ue(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).

[mildsunrise]

More issues that I have:

• GolombExpDecoder::Decode() invokes UB with certain user input.
• BitWritter::Put() does not signal error if there isn't enough space in buffer (at least, not immediately).
• It is possible to underflow cached, for example by reading more bits than present. this can then cause Left() to underflow as well.

[bcostdolby]

Overall this class makes some basic assumptions on its usage that are not enforced which seems to allow these kinds of issues (especially if needing to decode some kind of variable amount of bits).

It seems there is an "error" state on the class which seems to be its way of notifying issues to the caller. Should we also set that in all these cases (in addition to preventing UB)?

I would think we need to enforce the value passed to this is always <=32 in general for all the public functions.

For example even the Get() does a: ret = cache >> (32 - n)

if n is given as larger than 32 this is also UB right?

I would suggest a common check maybe for all public functions (seems they are all public) for places like:

• Get(n) : Where n > 32
• Check(n) : Where n > 32 (I guess covered by the get, but still) and others...:
• Skip(n)
• Peek(n)
• SkipCached()
• GetCached()

seems we are good at checking n == 0 but not n > 32.

[bcostdolby]

Also GetNonSymmetric() could probably use some comments describing what it is trying to do. The name doesnt really tell us much.

[mildsunrise]

many of the tasks described in the comments of this PR, as well as outstanding comments in diffs, have been addressed now.

I'll make a list of the ones that haven't been addressed yet (please everyone verify I'm not missing anything):

• GolombExpDecoder::Decode() invokes UB with certain user input
• BitWritter::Put() does not signal error if there isn't enough space in buffer (at least, not immediately)
• check for n>32 in Check() too, requested by @bcostdolby (actually I'd change all these checks into asserts :smiling_imp: but I understand if others don't wanna)
• GetNonSymmetric() could probably use some comments describing what it is trying to do

some questions:

It seems there is an "error" state on the class which seems to be its way of notifying issues to the caller. Should we also set that in all these cases (in addition to preventing UB)?

regarding this, I think we should do the opposite, and convert this file to use exceptions. not sure how others see it

in any case, I'd merge this now and we can open a new PR for further fixes. also, @bcostdolby you mentioned you added things to "overall ticket", which ticket is that?

[murillo128]

+1 to replace error flags by proper exception handling.

Not sure about asserts, I would be worried about crashing on production because a software bug.

El mar, 27 ago 2024, 15:57, Alba Mendez @.***> escribió:

many of the tasks described in the comments of this PR, as well as outstanding comments in diffs, have been addressed now.

I'll make a list of the ones that haven't been addressed yet (please everyone verify I'm not missing anything):

• GolombExpDecoder::Decode() invokes UB with certain user input
• BitWritter::Put() does not signal error if there isn't enough space in buffer (at least, not immediately)
• check for n>32 in Check() too, requested by @bcostdolby https://github.com/bcostdolby (actually I'd change all these checks into asserts 😈 but I understand if others don't wanna)
• GetNonSymmetric() could probably use some comments describing what it is trying to do

some questions:

It seems there is an "error" state on the class which seems to be its way of notifying issues to the caller. Should we also set that in all these cases (in addition to preventing UB)?

regarding this, I think we should do the opposite, and convert this file to use exceptions. not sure how others see it

in any case, I'd merge this now and we can open a new PR for further fixes. also, @bcostdolby https://github.com/bcostdolby you mentioned you added things to "overall ticket", which ticket is that?

— Reply to this email directly, view it on GitHub https://github.com/medooze/media-server/pull/257#issuecomment-2312640776, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIFN43XO65M5PMSMOCRI2DZTSAUFAVCNFSM6AAAAABDVZTDX2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJSGY2DANZXGY . You are receiving this because you commented.Message ID: @.***>