From 0d08232c465eaeea532899015c3ae2f3fc13619a Mon Sep 17 00:00:00 2001 From: Philip de Nier Date: Fri, 14 Jun 2024 16:47:54 +0100 Subject: [PATCH 1/5] Fix memory leak for header metadata extension factories --- deps/libMXFpp/libMXF++/HeaderMetadata.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/deps/libMXFpp/libMXF++/HeaderMetadata.cpp b/deps/libMXFpp/libMXF++/HeaderMetadata.cpp index b4e781f1..2a0052f4 100644 --- a/deps/libMXFpp/libMXF++/HeaderMetadata.cpp +++ b/deps/libMXFpp/libMXF++/HeaderMetadata.cpp @@ -117,8 +117,12 @@ void HeaderMetadata::registerObjectFactory(const mxfKey *key, AbsMetadataSetFact _objectFactory.insert(pair(*key, factory)); if (result.second == false) { - // replace existing factory with new one + // erase and delete the existing factory + AbsMetadataSetFactory *existing_factory = result.first->second; _objectFactory.erase(result.first); + delete existing_factory; + + // insert the new factory _objectFactory.insert(pair(*key, factory)); } } From 55729ff98e733627fb618e4cbcc5f2436cad7efc Mon Sep 17 00:00:00 2001 From: Philip de Nier Date: Fri, 14 Jun 2024 16:49:26 +0100 Subject: [PATCH 2/5] test: fix missing delete in test essence creation utility --- test/create_test_essence.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/create_test_essence.cpp b/test/create_test_essence.cpp index 8d54c9de..9c6ff6d6 100644 --- a/test/create_test_essence.cpp +++ b/test/create_test_essence.cpp @@ -430,6 +430,8 @@ static void write_d10(FILE *file, int type, unsigned int duration) unsigned int i; for (i = 0; i < duration; i++) write_buffer(file, data, frame_size); + + delete [] data; } static void write_mpeg2lg(FILE *file, int type, unsigned int duration, bool low_delay, bool closed_gop, From 9348db274fc05d0f08226b5c34ea8c0138a9cfd0 Mon Sep 17 00:00:00 2001 From: Philip de Nier Date: Fri, 14 Jun 2024 16:53:23 +0100 Subject: [PATCH 3/5] Add guards for NULL data ptr pass to fread/fwrite Only required in places where the origin of data is unknown and therefore could be provided by a library user. This avoids a sanitizer warning about the argument being flagged as non-NULL when there is a test that passes NULL. GCC appears to be fine with it but other compilers may not as the behaviour is undefined. --- deps/libMXF/mxf/mxf_file.c | 30 ++++++++++++++++++++++++------ deps/libMXF/mxf/mxf_page_file.c | 30 ++++++++++++++++++++++++------ src/common/BMXFileIO.cpp | 26 ++++++++++++++++++++------ 3 files changed, 68 insertions(+), 18 deletions(-) diff --git a/deps/libMXF/mxf/mxf_file.c b/deps/libMXF/mxf/mxf_file.c index 1403df3d..ddc43f61 100644 --- a/deps/libMXF/mxf/mxf_file.c +++ b/deps/libMXF/mxf/mxf_file.c @@ -123,18 +123,36 @@ static void disk_file_close(MXFFileSysData *sysData) static uint32_t disk_file_read(MXFFileSysData *sysData, uint8_t *data, uint32_t count) { char errorBuf[128]; - uint32_t result = (uint32_t)fread(data, 1, count, sysData->file); - if (result != count && ferror(sysData->file)) - mxf_log_error("fread failed: %s\n", mxf_strerror(errno, errorBuf, sizeof(errorBuf))); + uint32_t result; + + if (data) { + result = (uint32_t)fread(data, 1, count, sysData->file); + if (result != count && ferror(sysData->file)) + mxf_log_error("fread failed: %s\n", mxf_strerror(errno, errorBuf, sizeof(errorBuf))); + } else { + result = 0; + if (result != count) + mxf_log_error("fread failed: passed NULL data ptr\n"); + } + return result; } static uint32_t disk_file_write(MXFFileSysData *sysData, const uint8_t *data, uint32_t count) { char errorBuf[128]; - uint32_t result = (uint32_t)fwrite(data, 1, count, sysData->file); - if (result != count) - mxf_log_error("fwrite failed: %s\n", mxf_strerror(errno, errorBuf, sizeof(errorBuf))); + uint32_t result; + + if (data) { + result = (uint32_t)fwrite(data, 1, count, sysData->file); + if (result != count) + mxf_log_error("fwrite failed: %s\n", mxf_strerror(errno, errorBuf, sizeof(errorBuf))); + } else { + result = 0; + if (result != count) + mxf_log_error("fwrite failed: passed NULL data ptr\n"); + } + return result; } diff --git a/deps/libMXF/mxf/mxf_page_file.c b/deps/libMXF/mxf/mxf_page_file.c index 0c771a20..bf477c8f 100644 --- a/deps/libMXF/mxf/mxf_page_file.c +++ b/deps/libMXF/mxf/mxf_page_file.c @@ -129,18 +129,36 @@ static void disk_file_close(FileDescriptor *fileDesc) static uint32_t disk_file_read(FileDescriptor *fileDesc, uint8_t *data, uint32_t count) { char errorBuf[128]; - uint32_t result = (uint32_t)fread(data, 1, count, fileDesc->file); - if (result != count && ferror(fileDesc->file)) - mxf_log_error("fread failed: %s\n", mxf_strerror(errno, errorBuf, sizeof(errorBuf))); + uint32_t result; + + if (data) { + result = (uint32_t)fread(data, 1, count, fileDesc->file); + if (result != count && ferror(fileDesc->file)) + mxf_log_error("fread failed: %s\n", mxf_strerror(errno, errorBuf, sizeof(errorBuf))); + } else { + result = 0; + if (result != count) + mxf_log_error("fread failed: passed NULL data ptr\n"); + } + return result; } static uint32_t disk_file_write(FileDescriptor *fileDesc, const uint8_t *data, uint32_t count) { char errorBuf[128]; - uint32_t result = (uint32_t)fwrite(data, 1, count, fileDesc->file); - if (result != count) - mxf_log_error("fwrite failed: %s\n", mxf_strerror(errno, errorBuf, sizeof(errorBuf))); + uint32_t result; + + if (data) { + result = (uint32_t)fwrite(data, 1, count, fileDesc->file); + if (result != count) + mxf_log_error("fwrite failed: %s\n", mxf_strerror(errno, errorBuf, sizeof(errorBuf))); + } else { + result = 0; + if (result != count) + mxf_log_error("fwrite failed: passed NULL data ptr\n"); + } + return result; } diff --git a/src/common/BMXFileIO.cpp b/src/common/BMXFileIO.cpp index e20726f4..55c6a556 100644 --- a/src/common/BMXFileIO.cpp +++ b/src/common/BMXFileIO.cpp @@ -80,9 +80,16 @@ BMXFileIO::~BMXFileIO() uint32_t BMXFileIO::Read(unsigned char *data, uint32_t size) { - size_t result = fread(data, 1, size, mFile); - if (result != size && ferror(mFile)) - log_error("Failed to read %u bytes: %s\n", size, bmx_strerror(errno).c_str()); + size_t result; + if (data) { + result = fread(data, 1, size, mFile); + if (result != size && ferror(mFile)) + log_error("Failed to read %u bytes: %s\n", size, bmx_strerror(errno).c_str()); + } else { + result = 0; + if (result != size) + log_error("Failed to read %u bytes: passed NULL data ptr\n", size); + } return (uint32_t)result; } @@ -91,9 +98,16 @@ uint32_t BMXFileIO::Write(const unsigned char *data, uint32_t size) { BMX_ASSERT(!mReadOnly); - size_t result = fwrite(data, 1, size, mFile); - if (result != size) - log_error("Failed to write %u bytes: %s\n", size, bmx_strerror(errno).c_str()); + size_t result; + if (data) { + result = fwrite(data, 1, size, mFile); + if (result != size) + log_error("Failed to write %u bytes: %s\n", size, bmx_strerror(errno).c_str()); + } else { + result = 0; + if (result != size) + log_error("Failed to write %u bytes: passed NULL data ptr\n", size); + } return (uint32_t)result; } From e6cd47571e379cc9243bbcbf960b4486cf7c3e77 Mon Sep 17 00:00:00 2001 From: Philip de Nier Date: Fri, 14 Jun 2024 16:57:57 +0100 Subject: [PATCH 4/5] Add check before memcpy for source data not NULL --- deps/libMXF/mxf/mxf_page_file.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/deps/libMXF/mxf/mxf_page_file.c b/deps/libMXF/mxf/mxf_page_file.c index bf477c8f..0ace367b 100644 --- a/deps/libMXF/mxf/mxf_page_file.c +++ b/deps/libMXF/mxf/mxf_page_file.c @@ -360,7 +360,8 @@ static Page* open_page(MXFFileSysData *sysData, int64_t position) Page *newPages; CHK_MALLOC_ARRAY_ORET(newPages, Page, sysData->numPagesAllocated + PAGE_ALLOC_INCR); - memcpy(newPages, sysData->pages, sizeof(Page) * sysData->numPagesAllocated); + if (sysData->pages) + memcpy(newPages, sysData->pages, sizeof(Page) * sysData->numPagesAllocated); SAFE_FREE(sysData->pages); sysData->pages = newPages; sysData->numPagesAllocated += PAGE_ALLOC_INCR; From d5ac6c52b5e1d17ffb02436f2b7daafe51e493e7 Mon Sep 17 00:00:00 2001 From: Philip de Nier Date: Fri, 14 Jun 2024 16:59:53 +0100 Subject: [PATCH 5/5] Add cast to avoid warning about shifting 24 bits --- deps/libMXF/mxf/mxf_file.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/deps/libMXF/mxf/mxf_file.c b/deps/libMXF/mxf/mxf_file.c index ddc43f61..e1433cae 100644 --- a/deps/libMXF/mxf/mxf_file.c +++ b/deps/libMXF/mxf/mxf_file.c @@ -440,10 +440,10 @@ int mxf_read_uint32(MXFFile *mxfFile, uint32_t *value) uint8_t buffer[4]; CHK_ORET(mxf_file_read(mxfFile, buffer, 4) == 4); - *value = (buffer[0] << 24) | - (buffer[1] << 16) | - (buffer[2] << 8) | - buffer[3]; + *value = ((uint32_t)buffer[0] << 24) | + ((uint32_t)buffer[1] << 16) | + ((uint32_t)buffer[2] << 8) | + (uint32_t)buffer[3]; return 1; }