Opened 14 months ago

Closed 13 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 14 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 14 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 14 months ago by Klaas de Waal

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

comment:1 Changed 14 months ago by David Hampton

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

comment:2 Changed 14 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 14 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 14 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 13 months ago by Klaas de Waal

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

comment:5 Changed 13 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.