Skip to content

Comments

Fix buffer overflow and memory leak in set method#30

Merged
rafaelkallis merged 3 commits intomainfrom
copilot/fix-key-insertion-bug
Nov 24, 2025
Merged

Fix buffer overflow and memory leak in set method#30
rafaelkallis merged 3 commits intomainfrom
copilot/fix-key-insertion-bug

Conversation

Copy link
Contributor

Copilot AI commented Nov 24, 2025

The set method had a buffer overflow writing one byte beyond allocated memory and a memory leak using the wrong delete operator for arrays.

Changes

Buffer overflow fix (line 126)

// Before: copied key_len + 1 bytes into key_len buffer
std::copy(key, key + key_len + 1, root_->prefix_);

// After: copy exactly key_len bytes
std::copy(key, key + key_len, root_->prefix_);

Memory leak fix (line 200)

// Before: wrong delete operator for array
delete old_prefix;

// After: use delete[] for arrays allocated with new[]
delete[] old_prefix;

Note on reported "aa\0" behavior

The issue description expected "aa" and "aa\0" to be distinct keys, but they're identical from the API's perspective:

art.set("aa", 3);    // strlen("aa") = 2, key_len = 3
art.set("aa\0", 10); // strlen("aa\0") = 2, key_len = 3 (stops at first null)
assert(art.get("aa") == 10); // Same key, value replaced

This is expected C-string semantics. The string literal "aa\0" contains ['a','a','\0','\0'], but strlen() stops at the first null. Supporting embedded nulls would require accepting explicit lengths, a breaking API change.

Added test case documenting this behavior.

Original prompt

This section details on the original issue you should resolve

<issue_title>Set key bug</issue_title>
<issue_description>```
art::art art;
art.set("aaa", 1);
art.set("aab", 2);
art.set("aa", 3);
art.set("aa\0", 10);
assert(art.get("aa") == 3);


This program will crush because of `assert(art.get("aa") == 3);` and the value of `aa` is  10.
While insert 'aa' into art , code logic is incorrect.</issue_description>

## Comments on the Issue (you are @copilot in this section)

<comments>
</comments>


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits November 24, 2025 08:23
Co-authored-by: rafaelkallis <9661903+rafaelkallis@users.noreply.github.com>
Co-authored-by: rafaelkallis <9661903+rafaelkallis@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix key insertion logic in art class Fix buffer overflow and memory leak in set method Nov 24, 2025
Copilot AI requested a review from rafaelkallis November 24, 2025 08:28
@rafaelkallis rafaelkallis marked this pull request as ready for review November 24, 2025 09:29
Copilot AI review requested due to automatic review settings November 24, 2025 09:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes two critical bugs in the set method: a buffer overflow writing beyond allocated memory and a memory leak from incorrect delete operator usage. The fixes ensure correct memory allocation/deallocation patterns and add test coverage documenting C-string semantics behavior.

  • Fixed buffer overflow by copying exactly key_len bytes instead of key_len + 1 into a key_len-sized buffer (line 126)
  • Fixed memory leak by using delete[] instead of delete for array deallocation (line 200)
  • Added test case documenting that "aa" and "aa\0" are identical keys due to C-string semantics

Reviewed changes

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

File Description
include/art/art.hpp Fixes buffer overflow in root node creation (line 126) and memory leak in prefix reallocation (line 200)
test/art.cpp Adds test case documenting C-string semantics for keys with explicit trailing nulls

@rafaelkallis rafaelkallis merged commit 3ee24a2 into main Nov 24, 2025
8 checks passed
@rafaelkallis rafaelkallis deleted the copilot/fix-key-insertion-bug branch November 24, 2025 09:43
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.

Set key bug

2 participants