Opened 2 years ago

Closed 2 years ago

Last modified 17 months ago

#13106 closed Bug Report - Memory Leak (fixed)

Mythtv Video Service API GetVideoByFileName memory leak

Reported by: rich@… Owned by: Bill Meek
Priority: minor Milestone: 0.28.2
Component: MythTV - Services API - Backend Version: 0.28.1
Severity: low Keywords:
Cc: Ticket locked: no

Description

The DTC::VideoMetadataInfo?* Video::GetVideoByFileName?( const QString &FileName? ) function in mythtv/programs/mythbackend/services/video.cpp can leak memory.

A VideoMetadataListManager? instance is heap allocated and afterwards the function can throw an exception without deleting the heap allocated instance. This can be seen in the function definition below:

TC::VideoMetadataInfo* Video::GetVideoByFileName( const QString &FileName )
{
    VideoMetadataListManager::metadata_list videolist;
    VideoMetadataListManager::loadAllFromDatabase(videolist);
    VideoMetadataListManager *mlm = new VideoMetadataListManager();
    mlm->setList(videolist);
    VideoMetadataListManager::VideoMetadataPtr metadata = mlm->byFilename(FileName);

    if ( !metadata )
        throw( QString( "No metadata found for selected filename!." ));

    DTC::VideoMetadataInfo *pVideoMetadataInfo = new DTC::VideoMetadataInfo();

    FillVideoMetadataInfo ( pVideoMetadataInfo, metadata, true );

    delete mlm;

    return pVideoMetadataInfo;
}

Change History (5)

comment:1 Changed 2 years ago by Bill Meek

Milestone: needs_triage0.28.2
Status: newaccepted

comment:2 Changed 2 years ago by Bill Meek <bmeek@…>

Resolution: fixed
Status: acceptedclosed

In 777965c9108c20b1956cf5cab2d060abcab02bf0/mythtv:

Services API: Fixes #13106 (memory leak)

comment:3 Changed 2 years ago by Bill Meek <bmeek@…>

In eea73b70530c5674167cbd1d6608b4e4d97e5259/mythtv:

Services API: Fixes #13106 (memory leak)

(cherry picked from commit 777965c9108c20b1956cf5cab2d060abcab02bf0)

comment:4 Changed 2 years ago by Jonatan Lindblad

The fix itself looks alright, but I have to question the original author's use of heap allocation here.

VideoMetadataListManager only contains a pointer member and is only used temporarily so I think we should use the stack here.

It is quite inconsistent, VideoMetadataListManager::metadata_list is three times larger than VideoMetadataListManager it seems, but videolist is (correctly in my opinion) in allocated on the stack.

It probably doesn't matter very much, it just caught my eye.

comment:5 Changed 17 months ago by Bill Meek

Owner: changed from Bill Meek to Bill Meek
Note: See TracTickets for help on using tickets.