Skip to content

LoadGpx: embedded NUL byte silently truncates the file #67

Description

@thomthom

Split out of #50, which fixed the same bug in ParseGpx only.

Problem

LoadGpx in src/cpp/fastgpx/fastgpx.cpp hands the path to pugixml:

pugi::xml_parse_result result = doc.load_file(path.wstring().c_str());

pugixml reads the file and then runs the same parser that ParseGpx uses, and that parser
zero-terminates its own copy of the buffer and uses NUL as the end sentinel:

// save last character and make buffer zero-terminated (speeds up parsing)
char_t endch = buffer[length - 1];
buffer[length - 1] = 0;

So a NUL byte anywhere in the file ends the document as far as pugixml is concerned, and
everything after it is dropped. When the prefix happens to be well-formed, load() returns a
successfully parsed but truncated Gpx with no error - a corrupted or partially written file
silently loses tracks.

U+0000 is excluded from the XML Char production, so such a file is not well-formed and a
conforming parser is required to reject it. This is not a leniency decision.

Why it was not fixed with #50

#50 added an explicit scan in ParseGpx, which already receives the whole document as a
std::string:

if (const auto nul = data.find('\0'); nul != std::string::npos)
{
  throw parse_error(...);
}

LoadGpx never sees the bytes - pugixml opens and reads the file itself - so the same check
cannot be added without changing how files are read.

Options

  • Read the file into a buffer in LoadGpx and route it through the same path as ParseGpx, so
    both entry points get the check. This is the straightforward fix, but it changes the memory
    profile: pugixml's load_file streams into its own buffer, whereas this would hold the file
    contents and the DOM at the same time. Worth measuring against
    fastgpx_test "[!benchmark][parse]", which covers LoadGpx.
  • Reuse fastgpx::open_file from filesystem.cpp for the read, which would give open_file in filesystem.cpp is unused #24 an answer
    other than deleting it.
  • Leave load() as-is and document that file input is not checked. Inconsistent with parse(),
    so only worth it if the read costs more than the bug is worth.

Note that #23 (streaming scanner) would replace the file reading entirely, so a large
restructuring here may be wasted work; the cheap version is likely the right call.

Reproducing

Any GPX file with a NUL byte after a well-formed prefix, e.g. a complete document followed by
\0 and more content, loads as the prefix alone with no error. The equivalent input through
fastgpx.parse() raises fastgpx.ParseError after #50.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions