Opened 7 years ago

Closed 3 years ago

#10067 closed Patch - Bug Fix (Fixed)

Potential memory leak in EITHelper

Reported by: mythtv@… Owned by: Stuart Auchterlonie
Priority: minor Milestone: 0.28
Component: MythTV - EIT Version: Master Head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

In EITHelper.cpp - EITHelper keeps a list of incomplete_events, and unmatched_etts. If for some reason when parsing the EIT and ETT tables, it does not manage to match any ETT to an EIT entry, the unmatched item will remain on the list indefinitely.

incomplete_events stores character strings allocated with new. If the EITHelper is destroyed, those will be unreferenced and simply leaked.

If EITHelper is not destroyed, incomplete_events and unmatched_etts can grow without bounds. Aside from the memory consumption, that has another unwanted side effect that leftover unmatched items from earlier calls can be picked up as incorrect matches on later calls. This will result in program descriptions being updated with descriptions from other shows - a behavior I'm seeing, which is the whole reason I started looking at EITHelper.

Evidence of the leak can be seen by turning on verbose logging for EIT and watching for this log message:

        VERBOSE(VB_EIT, LOC +
                QString("Added %1 events -- complete(%2) "
                        "incomplete(%3) unmatched(%4)")
                .arg(insertCount).arg(db_events.size())
                .arg(incomplete_events.size()).arg(unmatched_etts.size()));

In the same method, there is a logic bug when iterating over events. The for loop compares the index with the size of the db_events queue, but at the same time the loops is removing events. Thus the index is incremented and the size is decreased, so the loop will always end with only half of the events processed.

So this:

    for (uint i = 0; (i < kChunkSize) && (i < db_events.size()); i++)
    {
        DBEventEIT *event = db_events.dequeue();

should become this:

    for (uint i = 0; (i < kChunkSize) && (db_events.size() > 0); i++)
    {
        DBEventEIT *event = db_events.dequeue();

I don't think an ETT table from one event can legitimately be related to an EIT from a different event, so I believe after each iteration of that loop, incomplete_events and unmatched_etts should be cleared - taking care to free the memory associated with items stored in incomplete_events. }}}

Change History (7)

comment:1 Changed 6 years ago by stuartm

Milestone: unknown0.25
Version: UnspecifiedMaster Head

comment:2 Changed 6 years ago by Github

Refs #10067. Fix event dequeing logic issue

Branch: master Changeset: d6f038c56b52a8fda05f13c6be897852cb1b5953

comment:3 Changed 6 years ago by Stuart Auchterlonie

Milestone: 0.250.26
Status: newassigned

The second part of this, to remove incomplete events and unmatched_etts can wait till 0.26 as I have no means of testing this one.

Stuart

comment:4 Changed 6 years ago by Kenni Lund [kenni a kelu dot dk]

Milestone: 0.260.26.1

comment:5 Changed 5 years ago by paulh

Milestone: 0.26.10.28

comment:6 Changed 5 years ago by Ken <kenkyee@…>

It's fairly easy to reproduce w/ a dual HDHomerun ATSC tuner. I see this random EIT description quirk fairly often after leaving the MythTV box on for a week or more...

Not sure it should be pushed off to 0.28...

comment:7 Changed 3 years ago by dekarl

Resolution: Fixed
Status: assignedclosed

Closing this as the second part has been superseded by #11739. Marking as fixed for the first part.

Ken, its been pushed as us DVB-country-devs (who do the most work around EIT) are having a hard time testing stuff that only applies to ATSC-countries.

Note: See TracTickets for help on using tickets.