Fix/fix graph node names that can become unterminated - #69
Conversation
Added a test for maximum-length node names in the graph.
Added non-zero data initialization for node name to detect missing null terminator.
srpatcha
left a comment
There was a problem hiding this comment.
Verified — and the test is the good part
strncpy writes no terminator when the source fills the buffer, so a node name of exactly EOS_MAX_NAME - 1 characters left n->name unterminated and every later read ran into whatever followed it in the struct. Correct fix.
What makes this a real regression guard rather than a restatement of the one-line change:
/* Fill the destination with non-zero data so the test can detect
* a missing null terminator even though eos_graph_init() zeroes
* the graph initially. */
memset(g.nodes[0].name, 'X', EOS_MAX_NAME);Without that, eos_graph_init() has already zeroed the struct, so the byte after the copy is \0 by accident and the test passes against the unfixed code. Pre-poisoning the buffer is exactly what makes the assertion mean something. That is the step most strncpy fixes skip.
Merge order
This is currently red, but not because of anything in it — origin/master has a SyntaxError in ebuild/build/dispatch.py that makes the whole suite uncollectable:
$ pytest
2 errors during collection
Merged on top of #66, which repairs that: 206 passed. So this needs #66 to land first, then it goes in as-is. Nothing to change here.
Verification
Merged onto origin/master + #66 locally; pytest 206 passed. The graph test was also run against the unfixed graph.c to confirm it fails there.
Issue
Graph node names are stored in a fixed-size character buffer. When a node name reaches the maximum supported length,
strncpy()may copy the requested number of characters without adding a null terminator.This can leave
EosNode.nameunterminated even though it is later used as a C string by functions such asstrcmp()and logging/output code. This can result in incorrect string handling and potentially undefined behavior.Proposed Approach
Ensure that the destination buffer is explicitly null terminated after copying the node name.
The implementation uses:
A regression test was also added for a node name that uses the maximum number of characters available before the null terminator.
The test pre-fills the destination buffer with non-zero data before adding the node. This ensures the test can detect a missing null terminator rather than accidentally passing because the graph was initially zero-initialized.
Testing
Added
test_graph_max_length_node_name()to the graph test suite.The test verifies that:
eos_graph_find_node().The existing graph tests for initialization, node creation, node lookup, topological sorting, cycle detection, and independent nodes are also retained.
Validation
The test specifically exercises the boundary condition where the node name contains
EOS_MAX_NAME - 1characters.The destination buffer is deliberately filled with non-zero bytes before adding the node. This allows the regression test to detect a missing null terminator if the explicit termination is removed from the implementation.
Considerations and Limitations
This change does not alter the existing public API or the fixed-size node-name representation.
Names longer than
EOS_MAX_NAME - 1characters continue to be truncated to fit the destination buffer. The important difference is that the resulting stored name is guaranteed to be null terminated.The change is intentionally limited to node-name handling and its regression test.