RDKEMW-24183 : Fix coverity issues (from copilot) in subttxrend-ttml

[krithikasvraman]

Reason for change: Fix coverity issues in subttxrend-ttml
Test Procedure: Regression tests
Risks: Low
Signed-off-by:krithika_venkataraman@comcast.com

[Copilot]

Pull request overview

This PR targets Coverity/static-analysis findings in the subttxrend-ttml component, focusing on safer container usage, improved parser diagnostics, and more robust file I/O in the dumper utilities.

Changes:

• Fix SAX attribute collection by reserving capacity and emplace_back-ing attributes instead of pre-sizing the vector.
• Add basic logging for XML parse errors and warnings.
• Improve dump-file writing (binary append, detect short writes) and handle ftell() failure when reading TTML from file.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
subttxrend-ttml/src/Parser/XmlLibSaxParserWrapper.cpp	Adjusts attribute vector construction to avoid unintended pre-sized/default entries.
subttxrend-ttml/src/Parser/Parser.cpp	Adds error/warning logging callbacks for XML parsing.
subttxrend-ttml/src/DataDumper.cpp	Fixes file open mode, adds short-write detection, and checks ftell() failure before resizing buffers.

Suppressed comments (1)

subttxrend-ttml/src/Parser/Parser.cpp:191

• onWarning(const char* msg, ...) ignores its variadic arguments; if msg contains format specifiers the log output will be misleading. Format the message using va_list/vsnprintf (or a logger vprintf-style API) prior to logging.

void Parser::onWarning(const char * msg, ...)
{
    m_logger.ostrace(__LOGGER_FUNC__, " xml parse warning: ", msg ? msg : "");
}

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

subttxrend-ttml/src/DataDumper.cpp:130

• After ftell() succeeds, fileSize is a signed type; data.resize(fileSize) relies on an implicit signed→unsigned conversion and the code currently logs a successful read even when fread() returns fewer bytes than expected. Casting once to std::size_t and shrinking/logging on short reads prevents returning partially-filled buffers as if they were complete.

       }
       fseek(dataFile,0,SEEK_SET);

       data.resize(fileSize);
       auto dataRead = fread(data.data(), 1, data.size(), dataFile);

[Copilot]

🟡 Changes recommended

DataDumper::toFile() uses append mode for writing complete dump payloads, which can corrupt outputs when the same filename is reused.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

A few correctness/operational edge cases remain (signed-to-unsigned reserve risk in SAX handling, and binary dump logging / binary read mode portability) that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

subttxrend-ttml/src/Parser/XmlLibSaxParserWrapper.cpp:122

• nb_attributes is an int; if it ever comes in negative, casting to size_t will produce a huge value and reserve() may attempt a massive allocation even though the loop won’t run. Guard the reserve with a > 0 check (or clamp to 0) before converting to size_t.

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There are still unchecked/unsafe error paths (notably fseek() error handling and a potential negative-to-size_t cast in reserve()) that can lead to incorrect behavior or large allocations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

readTtmlFromFile() still returns a buffer sized to the expected file length even when fread() reads fewer bytes, which can propagate corrupted TTML data to callers.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

subttxrend-ttml/src/DataDumper.cpp:163

• readTtmlFromFile() resizes the output vector to fileSize before calling fread(), but it doesn't handle a short read. If fread() returns fewer bytes than expected, the returned buffer will include trailing zero-bytes (from resize) and callers will parse corrupted TTML. Consider logging an error and resizing the vector down to dataRead (or returning empty) when dataRead != expectedSize.

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

readTtmlFromFile() can return buffers with trailing zero bytes on short reads, and toFile() logs raw binary buffer contents, both of which can cause incorrect behavior and operational issues.

Review details

Suppressed comments (2)

subttxrend-ttml/src/DataDumper.cpp:105

• toFile() builds a std::string from the raw buffer and logs it with %s. This is expensive for large dumps and incorrect/unsafe for binary payloads (e.g., PNGs) because embedded NULs/non-printable bytes will truncate/garble logs and can flood logging output. Log only metadata (size/path) here.

        auto bufferStr = std::string{(char const*)buffer, bufferLen};
        g_logger.info("dumping: %s (size %zu) to %s", bufferStr.c_str(), bufferLen, filename.c_str());

subttxrend-ttml/src/DataDumper.cpp:165

• readTtmlFromFile() resizes the vector to fileSize before fread(), but if fread() returns a short read the function still returns a buffer of the original size (with trailing zero bytes). That can corrupt downstream parsing; handle short reads by logging and resizing to the actual number of bytes read (or treating it as an error).

       data.resize(fileSize);
       auto dataRead = fread(data.data(), 1, data.size(), dataFile);

       g_logger.info("read: %zu out of %zu from %s", dataRead, data.size(), path.c_str());

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[jamcoole]

🔵 Needs a closer look

readTtmlFromFile() can return buffers with trailing zero bytes on short reads, and toFile() logs raw binary buffer contents, both of which can cause incorrect behavior and operational issues.

Review details

[jamcoole]

sorry closed this by accident, re-opened