Skip to content

bagFreeMetadata() never frees metadata.characterSet — every init/free cycle leaks #147

Description

@CrJyyy

Summary

bagInitMetadata() allocates metadata.characterSet, but bagFreeMetadata() does not free it.
Every bagInitMetadata() / bagFreeMetadata() pair — and therefore every BAG::Metadata
construction and destruction — leaks that string.

A fifteen-line program using nothing but the two public C API functions is enough:

#include "bag_metadatatypes.h"
#include <cstdio>

int main()
{
    for (int i = 0; i < 3; ++i) {
        BagMetadata m;
        bagInitMetadata(m);
        bagFreeMetadata(m);
    }
    printf("3 init/free cycles done\n");
    return 0;
}
$ g++ -std=c++17 -g -fsanitize=address -Iapi -o repro repro.cpp api/bag_metadatatypes.cpp
$ ./repro
=================================================================
==2191063==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 15 byte(s) in 3 object(s) allocated from:
    #0 operator new[](unsigned long)
    #1 copyString        api/bag_metadatatypes.cpp:28
    #2 bagInitMetadata   api/bag_metadatatypes.cpp:426
    #3 main              repro.cpp:11

SUMMARY: AddressSanitizer: 15 byte(s) leaked in 3 allocation(s).

Five bytes per cycle — "utf8" plus its terminator. No HDF5, no XML, no file I/O is involved;
bag_metadatatypes.cpp only #includes <H5Cpp.h> without using it, so an empty stub header is
enough to build that translation unit on its own.

Version

master at 3d4c1c0 ("BAG 2.0.5 release", 2026-02-05).

Cause

bagInitMetadata() allocates six strings:

// api/bag_metadatatypes.cpp:420
void bagInitMetadata(BagMetadata& metadata)
{
    metadata.fileIdentifier = nullptr;
    metadata.dateStamp = nullptr;
    metadata.language = copyString("en");
    metadata.characterSet = copyString("utf8");            // <-- line 426
    metadata.hierarchyLevel = copyString("dataset");
    metadata.metadataStandardName = copyString("ISO 19115");
    metadata.metadataStandardVersion = copyString("2003/Cor.1:2006");
    ...

bagFreeMetadata() frees five of them:

// api/bag_metadatatypes.cpp:464
void bagFreeMetadata(BagMetadata& metadata) noexcept
{
    delete[] metadata.fileIdentifier;
    delete[] metadata.dateStamp;
    delete[] metadata.language;
    delete[] metadata.hierarchyLevel;              // characterSet is missing here
    delete[] metadata.metadataStandardName;
    delete[] metadata.metadataStandardVersion;
    ...

characterSet is never released. Grepping the tree, the only delete[] metadata.characterSet is
api/bag_metadata_import.cpp:1675, and that one is a replace-before-overwrite inside the V2 XML
import, not a release. (api/bag_metadatatypes.cpp:125 frees
dataIdentificationInfo.characterSet, a different member of a different struct.)

BAG::Metadata::~Metadata() calls bagFreeMetadata(), so C++ users inherit the leak — that is how
this surfaced, as ~41 MB across 8.2 M objects in a long-running process that repeatedly built
BAG::Metadata objects.

Possible fix

--- a/api/bag_metadatatypes.cpp
+++ b/api/bag_metadatatypes.cpp
@@ -466,6 +466,7 @@ void bagFreeMetadata(BagMetadata& metadata) noexcept
     delete[] metadata.fileIdentifier;
     delete[] metadata.dateStamp;
     delete[] metadata.language;
+    delete[] metadata.characterSet;
     delete[] metadata.hierarchyLevel;
     delete[] metadata.metadataStandardName;
     delete[] metadata.metadataStandardVersion;

With that applied the reproducer above exits 0 with LeakSanitizer clean.

A second, related leak in the legacy import path

While reading that code I noticed the same three fields being overwritten without release in the
smXML import, which leaks whatever bagInitMetadata() put there:

// api/bag_metadata_import.cpp:1481   (legacy smXML path)
    //gmd:language
    metadata.language = getContentsAsCharStar(*pRoot, "/smXML:MD_Metadata/language");

    //gmd:characterSet
    metadata.characterSet = copyString("eng");

    //gmd:hierarchyLevel
    metadata.hierarchyLevel = copyString("dataset");

The V2 path a couple of hundred lines below does the same three assignments correctly:

// api/bag_metadata_import.cpp:1669
    delete[] metadata.language;
    metadata.language = getContentsAsCharStar(*pRoot, ".../gmd:LanguageCode");

    delete[] metadata.characterSet;
    metadata.characterSet = getContentsAsCharStar(*pRoot, ".../gmd:MD_CharacterSetCode");

    delete[] metadata.hierarchyLevel;
    metadata.hierarchyLevel = getContentsAsCharStar(*pRoot, ".../gmd:MD_ScopeCode");

Adding the three missing delete[] lines to the legacy path would make the two consistent. I have
not built this path locally (no libxml2 development headers on this machine), so this part is from
reading the source — but it matches what was observed: alongside the characterSet leak there were
75 leaked 8-byte "dataset" allocations from bagInitMetadata(), and hierarchyLevel is freed
by bagFreeMetadata(), so the only way to lose it is an overwrite like the one above.

How it was found

Automated fuzzing around BAG::Dataset::create(). The leak was then reduced to the standalone
program above, which touches neither the fuzzer nor any file format, so nothing in this report
depends on the original harness.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions