Skip to content

Fix/fix graph node names that can become unterminated - #69

Open
Akhil498-coder wants to merge 3 commits into
embeddedos-org:masterfrom
Akhil498-coder:fix/Fix-graph-node-names-that-can-become-unterminated
Open

Fix/fix graph node names that can become unterminated#69
Akhil498-coder wants to merge 3 commits into
embeddedos-org:masterfrom
Akhil498-coder:fix/Fix-graph-node-names-that-can-become-unterminated

Conversation

@Akhil498-coder

Copy link
Copy Markdown

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.name unterminated even though it is later used as a C string by functions such as strcmp() 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:

strncpy(n->name, name, EOS_MAX_NAME - 1);
n->name[EOS_MAX_NAME - 1] = '\0';

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:

  • A maximum-length node name can be added successfully.
  • The stored node name is null terminated.
  • The maximum-length node can be found using 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 - 1 characters.

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 - 1 characters 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.

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 srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants