Opened 4 months ago

Closed 4 months ago

#13409 closed Patch - Bug Fix (fixed)

Valgrind error in dvbci.cpp

Reported by: Klaas de Waal Owned by: Klaas de Waal
Priority: minor Milestone: 31.0
Component: MythTV - DVB Version: Master Head
Severity: low Keywords: DVBCI CAM
Cc: Stuart Auchterlonie, gigem Ticket locked: no

Description

Running mythbackend with valgrind gives the following error message:

==16878== Thread 28 DVBCam:
==16878== Syscall param write(buf) points to uninitialised byte(s)
==16878==    at 0x8BB1D57: __libc_write (write.c:26)
==16878==    by 0x8BB1D57: write (write.c:24)
==16878==    by 0x6D32D72: cTPDU::Write(int) (dvbci.cpp:333)
==16878==    by 0x6D33909: cCiTransportConnection::SendTPDU(unsigned char, int, unsigned char const*) (dvbci.cpp:454)
==16878==    by 0x6D33DB4: cCiTransportConnection::SendData(int, unsigned char const*) (dvbci.cpp:525)
==16878==    by 0x6D353D3: cCiSession::SendData(int, int, unsigned char const*) (dvbci.cpp:858)
==16878==    by 0x6D38850: cCiDateTime::SendDateTime() (dvbci.cpp:1160)
==16878==    by 0x6D38ABE: cCiDateTime::Process(int, unsigned char const*) (dvbci.cpp:1180)
==16878==    by 0x6D3CC9C: cLlCiHandler::Process() (dvbci.cpp:1780)
==16878==    by 0x6D21913: DVBCam::run() (dvbcam.cpp:268)
==16878==    by 0x71AD306: MThread::run() (mthread.cpp:322)
==16878==    by 0x71ADC7F: MThreadInternal::run() (mthread.cpp:79)
==16878==    by 0x9D332FA: QThreadPrivate::start(void*) (qthread_unix.cpp:367)
==16878==  Address 0x36eda7f6 is on thread 28's stack
==16878==  in frame #2, created by cCiTransportConnection::SendTPDU(unsigned char, int, unsigned char const*) (dvbci.cpp:452)
==16878==  Uninitialised value was created by a stack allocation
==16878==    at 0x6D38473: cCiDateTime::SendDateTime() (dvbci.cpp:1133)
==16878==

This is caused by code in SendDateTime? that composes a 7-byte string with date and time in the format for the CAM.
The original code is this:

    struct tTime { unsigned short mjd; uint8_t h, m, s; short offset; };
     tTime T;
     T.mjd = htons(MJD);
     T.h = DEC2BCD(tm_gmt.tm_hour);
     T.m = DEC2BCD(tm_gmt.tm_min);
     T.s = DEC2BCD(tm_gmt.tm_sec);
     T.offset = static_cast<short>(htons(tm_loc.tm_gmtoff / 60));
...
     SendData(AOT_DATE_TIME, 7, (uint8_t*)&T);

The struct is filled with the data and then the address of the struct is passed on as a pointer to a byte array. The error is that the struct is 8 bytes long and not 7 bytes; there is a one byte hole between T.s and T.offset.
The result is that:

  • the hole between T.s and T.offset causes the Valgrind error message
  • the first byte of T.offset is sent on the position of the second byte
  • the second byte of T.offset is not sent

This can be fixed by using a byte array and copying the values to the correct position in the byte array, as shown here:

#define BYTE0(a) ((a) & 0xFF)
#define BYTE1(a) (((a) >> 8) & 0xFF)
     uint8_t T[7];
     uint16_t mjd = htons(MJD);
     int16_t local_offset = htons(tm_loc.tm_gmtoff / 60);
     T[0] = BYTE0(mjd);
     T[1] = BYTE1(mjd);
     T[2] = DEC2BCD(tm_gmt.tm_hour);
     T[3] = DEC2BCD(tm_gmt.tm_min);
     T[4] = DEC2BCD(tm_gmt.tm_sec);
     T[5] = BYTE0(local_offset);
     T[6] = BYTE1(local_offset);

This is a message sent to the CAM with the original code:

2019-02-19 16:34:47.294282 I  --> 00 01 a0 10 01 90 02 00 04 9f 84 41 07 e4 a5 15 34 47 00 00

This is a message sent to the CAM with the fix applied:

2019-02-19 17:06:09.019635 I  --> 00 01 a0 10 01 90 02 00 04 9f 84 41 07 e4 a5 16 06 09 00 3c

In this message, the last two bytes is the offset from local time to the UTC time in minutes; the three bytes before that is the UTC time.
With the fix the time is correct, as can be verified by comparing the log timestamp 17:06:09.019635 with the UTC time 16:06:09 and the offset of 60 minutes.

The patch is attached.

Attachments (2)

20190219-dvbci-datetime.patch (1.3 KB) - added by Klaas de Waal 4 months ago.
Fix offset of local time to UTC in DateTime? string to CAM
20190220-dvbci-datetime.patch (1.4 KB) - added by Klaas de Waal 4 months ago.
Fix offset of local time to UTC in DateTime? string to CAM, typecast in SendData? removed.

Download all attachments as: .zip

Change History (7)

Changed 4 months ago by Klaas de Waal

Fix offset of local time to UTC in DateTime? string to CAM

comment:1 Changed 4 months ago by David Hampton

Wouldn't it be easier to just attached the 'packed' attribute to the structure?

comment:2 Changed 4 months ago by Klaas de Waal

Yes, using the packed attribute would also work. However, attributes are implementation/compiler dependent. Also there is memory alignment; in this case a 16-bit value is accessed at an odd address if the struct is packed.
Copying the bytes to the exact location of a byte array always works independent of the compiler, the compiler flags, attributes and underlying hardware architecture.

comment:3 Changed 4 months ago by Klaas de Waal

The typecast in SendData?, to convert a pointer to the struct to a pointer to a byte array, is not needed when a byte array is used.
Attached is a new version of the patch in which also the typecast is removed.

Changed 4 months ago by Klaas de Waal

Fix offset of local time to UTC in DateTime? string to CAM, typecast in SendData? removed.

comment:4 Changed 4 months ago by Klaas de Waal

Milestone: needs_triage31.0
Owner: set to Klaas de Waal
Status: newaccepted

comment:5 Changed 4 months ago by Klaas de Waal <mythtv@…>

Resolution: fixed
Status: acceptedclosed

In ddaf1945c/mythtv:

Fixes #13409 - Valgrind error in dvbci.cpp

Signed-off-by: Klaas de Waal <kdewaal@…>

Note: See TracTickets for help on using tickets.