Opened 13 years ago

Closed 13 years ago

#1456 closed defect (fixed)

fixed underflow protection in PESPacket constructor

Reported by: Janne <janne-mythtv@…> 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)

pespacket-underflow-fix.patch (516 bytes) - added by Janne <janne-mythtv@…> 13 years ago.
pespacket-underflow-fix2.patch (1.3 KB) - added by Janne <janne-mythtv@…> 13 years ago.
assert.gdb.txt (2.6 KB) - added by Janne <janne-mythtv@…> 13 years ago.
assert2.gdb.txt (2.6 KB) - added by Janne <janne-mythtv@…> 13 years ago.
pespacket_debug.patch (738 bytes) - added by Janne <janne-mythtv@…> 13 years ago.
debugging patch for invalid pespackets
pespacket_debug.txt (8.0 KB) - added by Janne <janne-mythtv@…> 13 years ago.
first backtrace with debugging
pespacket_debug2.txt (2.9 KB) - added by Janne <janne-mythtv@…> 13 years ago.
another segfault with a non CRC packet
pespacket_debug3.txt (3.0 KB) - added by Janne <janne-mythtv@…> 13 years ago.
segfault with a packet with HasCRC()=true

Download all attachments as: .zip

Change History (18)

Changed 13 years ago by Janne <janne-mythtv@…>

comment:1 Changed 13 years ago by Janne <janne-mythtv@…>

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

buffer = "\"\000\000`\032É0ø\t\001!\0240Q(\235Ò'\030\000\000\002\000\000\000)M\023deu\016\005Lost in music\000i\003ò\234À\202\r19:0005.03#00(\236Ò' \000\000\002\000\000\000.M\030deu\023\005Sterne und Zeichen\000i\003ò\235@\202\r21:0005.03#00\212¥hWt\21220:25-20:40 Uhr (Wdh.)\212Vorteil Verbraucher - Der Ratgeber\21220:45-21:00"...

buffer = "OðÙn0Û\000\001\0041\000\001\001O½¶Ò$\b\000\000\003\000\000\200¾M\031deu\024\005FIGARO AM VORMITTAG\000P\fò\003\002deustereoNy\000deu\000s\00509:05  Fundbüro\21209:35  Musik und Information\21210:15  CD-Tipp\21210:45  Film-Tipp\21211:15  Das Thema\21211:45  Kalenderblatti\003ñ\032@_\004\000\000\000"...

Changed 13 years ago by Janne <janne-mythtv@…>

comment:2 Changed 13 years ago by Janne <janne-mythtv@…>

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 13 years ago by Janne <janne-mythtv@…>

Attachment: assert.gdb.txt added

Changed 13 years ago by Janne <janne-mythtv@…>

Attachment: assert2.gdb.txt added

comment:3 Changed 13 years ago by Janne <janne-mythtv@…>

added another backtrace with an assert

valgrind doesn show anything suspicious

comment:4 Changed 13 years ago by danielk

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 13 years ago by Janne <janne-mythtv@…>

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 13 years ago by Janne <janne-mythtv@…>

Attachment: pespacket_debug.patch added

debugging patch for invalid pespackets

comment:6 Changed 13 years ago by Janne <janne-mythtv@…>

first update, no crash yet but I see tons of PESPackets without CRC.

Changed 13 years ago by Janne <janne-mythtv@…>

Attachment: pespacket_debug.txt added

first backtrace with debugging

comment:7 Changed 13 years ago by Janne <janne-mythtv@…>

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 13 years ago by Janne <janne-mythtv@…>

Attachment: pespacket_debug2.txt added

another segfault with a non CRC packet

Changed 13 years ago by Janne <janne-mythtv@…>

Attachment: pespacket_debug3.txt added

segfault with a packet with HasCRC()=true

comment:8 Changed 13 years ago by Janne <janne-mythtv@…>

Daniel, as feared I get invalid packets with and without CRC.

comment:9 Changed 13 years ago by danielk

Status: newassigned

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 13 years ago by danielk

Resolution: fixed
Status: assignedclosed

(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...

Note: See TracTickets for help on using tickets.