Closed anatol closed 2 weeks ago
I am looking at this code https://github.com/libgit2/pygit2/blob/a9c7cce9eaff27b5c65bd23a35406b314c16922b/pygit2/utils.py#L112 and the comment says:
The above construct is still subject to FFI scoping rules, i.e. the
contents of 'struct' only remain valid within the StrArray context.
which makes me believe that in this block https://github.com/libgit2/pygit2/blob/a9c7cce9eaff27b5c65bd23a35406b314c16922b/pygit2/remotes.py#L270 pushopts
will be freed when the function is exited. Thus the test_push_options
uses invalid memory.
ping @kempniu
@anatol With what OS and CPU arch do you see this issue?
For context push options were implemented in PR #1282
I am using Linux at x86_64, AMD hardware.
Yeah, it looks like test_push_options()
is faulty indeed. I feared that while implementing it originally (see https://github.com/libgit2/pygit2/pull/1282#issuecomment-2018194432) and it seems we were just relying on favorable memory allocator behavior so far. Extending CI tests for pygit2 with an ASAN-enabled libgit2/pygit2 build would be great as that would catch issues like this in time.
Like I said originally in #1282, I don't have any bright ideas for testing this because we basically want to peek into a structure that only lives ephemerally inside pygit2/remotes.py:push()
and there is no infrastructure for having a server-side canary that could inspect things at the other end (server), which is what libgit2 has. Given all this, test_push_options()
should probably be removed :shrug:
Alternatively, to keep some form of limited testing for push options, just the assertions for string contents can be removed:
diff --git a/test/test_remote.py b/test/test_remote.py
index ee42535..dce094f 100644
--- a/test/test_remote.py
+++ b/test/test_remote.py
@@ -373,10 +373,7 @@ def test_push_options(mock_callbacks, origin, clone, remote):
remote.push(['refs/heads/master'], push_options=['foo'])
remote_push_options = mock_callbacks.return_value.push_options.remote_push_options
assert remote_push_options.count == 1
- assert ffi.string(remote_push_options.strings[0]) == b'foo'
remote.push(['refs/heads/master'], push_options=['Option A', 'Option B'])
remote_push_options = mock_callbacks.return_value.push_options.remote_push_options
assert remote_push_options.count == 2
- assert ffi.string(remote_push_options.strings[0]) == b'Option A'
- assert ffi.string(remote_push_options.strings[1]) == b'Option B'
This is safe to do because the git_push_options
structure contains the whole git_strarray remote_push_options;
structure, not just a pointer to it, so the count
member of remote_push_options
will remain within valid memory even outside of the StrArray
context manager.
Cool, would you do a PR?
Sure, see #1304.
I am porting my company system to the latest libgit2/pygit2 and I stuck due to a test error, a memory violation to be specific.
It happens in test_push_options() at this exact line
assert ffi.string(remote_push_options.strings[0]) == b'foo'
I ran the test with a debugger and I see that
remote_push_options
looks fine,remote_push_options.strings
is a valid object, butremote_push_options.strings[0]
points to some garbage address like0x2d2d2d2d2d2d2d
. When ffi.string() tries to dereference the address it crashes.So I ran the test with address sanitizer and I got
heap-use-after-free
error, i.e. address pointing byremote_push_options.strings[0]
been freed somewhere else.Here are the address stanitizer stack traces, though they are for the C stack rather than for python code: