Opened 18 years ago
Closed 18 years ago
#1456 closed defect (fixed)
fixed underflow protection in PESPacket constructor
Reported by: | Owned by: | danielk | |
---|---|---|---|
Priority: | minor | Milestone: | 0.20 |
Component: | dvb | Version: | head |
Severity: | medium | Keywords: | |
Cc: | Ticket locked: | no |
Description
Attached patch fixes the length calculation for _pesdataSize if Length() is 0.
Attachments (8)
Change History (18)
Changed 18 years ago by
Attachment: | pespacket-underflow-fix.patch added |
---|
comment:1 Changed 18 years ago by
Changed 18 years ago by
Attachment: | pespacket-underflow-fix2.patch added |
---|
comment:2 Changed 18 years ago by
Ok. something mysterious is going. I see absolutly bot why the assert is triggered.
see attached backtrace
#1459 is probably related to my problems
Changed 18 years ago by
Attachment: | assert.gdb.txt added |
---|
Changed 18 years ago by
Attachment: | assert2.gdb.txt added |
---|
comment:3 Changed 18 years ago by
added another backtrace with an assert
valgrind doesn show anything suspicious
comment:4 Changed 18 years ago by
Milestone: | → 0.20 |
---|---|
Version: | → head |
janne, can you check the HasCRC() flag of these packets with an invalid length? I'm guessing that they don't have a CRC and so we shouldn't be checking it...
comment:5 Changed 18 years ago by
I suspect too that the packets are bogus.
I test with the attached debugging patch and without my underflow patches. it will take some time since the crash happens unfortunatly not that often. It takes several hours.
Daniel i have to questions. First is this ctor only used by packets which should have a CRC and second are you aware of a change since somewhere in the 9220 or 9230 which may have caused this?
I have only one concern: If the packets are indeed bogus, than HasCRC() will by chance be in half of the packets be true.
Changed 18 years ago by
Attachment: | pespacket_debug.patch added |
---|
debugging patch for invalid pespackets
comment:6 Changed 18 years ago by
first update, no crash yet but I see tons of PESPackets without CRC.
comment:7 Changed 18 years ago by
first segfault.
I'm pretty sure that the packet with length 0 is the troublesome and has no CRC. It's _pesdataSize is 232-1 = ((uint32)0)-1 and it isn't segfaulting in the verifyCRC().
I will update the patch to log all HasCRC() values.
Changed 18 years ago by
Attachment: | pespacket_debug2.txt added |
---|
another segfault with a non CRC packet
Changed 18 years ago by
Attachment: | pespacket_debug3.txt added |
---|
segfault with a packet with HasCRC()=true
comment:9 Changed 18 years ago by
Status: | new → assigned |
---|
Ok, I will apply something similar to your patch, any table that says it has a CRC but doesn't have room for it according to the peslength should fail VerifyCRC().
But I'm also going to look at the signal monitor section code, I think we may need to do some basic table verification there.
comment:10 Changed 18 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
(In [9296]) Closes #1456, by adding a slightly modified version of Janne's patch for the CRC check on a packet with an invalid length.
When the CRC check is performed we do not know if the packet is valid, so it may contain an invalid length. This means we need to verify the length before performing the CRC check...
the updated patch fixes other uint underlows in pespacket.h
i think the length in the failing packet is bogus, but mythtv shluldn't fail.
two bogus buffer SIParser::ParseTable?() is called with