Fix refcounting in px_proxy_factory_copy (#280)#282
Merged
janbrummer merged 3 commits intolibproxy:mainfrom Apr 2, 2024
Merged
Fix refcounting in px_proxy_factory_copy (#280)#282janbrummer merged 3 commits intolibproxy:mainfrom
janbrummer merged 3 commits intolibproxy:mainfrom
Conversation
The second argument to g_boxed_copy() is meant to be a pointer to the boxed instance itself, not a pointer to a pointer to the boxed instance. For example, `g_boxed_copy (G_TYPE_STRV, environ)` is correct (equivalent to g_strdupv() in this case), but `g_boxed_copy (G_TYPE_STRV, &environ)` would be incorrect. The same principle applies here. The test passed despite this because self->pf and &self->pf happen to both be pointer-sized, and as a result of libproxy#280, the data that is pointed to was copied byte-by-byte but not otherwise used; but this will no longer be true when libproxy#280 is fixed, so the test needs fixing first. Signed-off-by: Simon McVittie <smcv@debian.org>
Otherwise there is no guarantee that the object will continue to exist after the original factory is destroyed, and both the copy and the original factory will try to unref it when destroyed, which is a programming error. Resolves: libproxy#280 Signed-off-by: Simon McVittie <smcv@debian.org>
Previously the copy was never freed, which hid the bugs fixed by the previous commits. Reproduces: libproxy#280 Signed-off-by: Simon McVittie <smcv@debian.org>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #282 +/- ##
==========================================
+ Coverage 72.67% 72.70% +0.03%
==========================================
Files 16 16
Lines 849 850 +1
Branches 241 241
==========================================
+ Hits 617 618 +1
Misses 137 137
Partials 95 95 ☔ View full report in Codecov by Sentry. |
Contributor
|
Ohh, there is a lot more broken in the current code. The last argument to G_DEFINE_BOXED_TYPE is the free function and i for some reason used it as new function... Could you please fix it as well? |
janbrummer
reviewed
Feb 28, 2024
Contributor
|
@smcv Are you going to update your PR soon? |
Contributor
|
Actually i'll merge it as it and will do a follow up PR. Thanks for your contribution! |
sir-xw
pushed a commit
to openkylin/libproxy
that referenced
this pull request
Apr 22, 2025
The second argument to g_boxed_copy() is meant to be a pointer to the boxed instance itself, not a pointer to a pointer to the boxed instance. For example, `g_boxed_copy (G_TYPE_STRV, environ)` is correct (equivalent to g_strdupv() in this case), but `g_boxed_copy (G_TYPE_STRV, &environ)` would be incorrect. The same principle applies here. The test passed despite this because self->pf and &self->pf happen to both be pointer-sized, and as a result of libproxy/libproxy#280, the data that is pointed to was copied byte-by-byte but not otherwise used; but this will no longer be true when #280 is fixed, so the test needs fixing first. Signed-off-by: Simon McVittie <smcv@debian.org> Forwarded: libproxy/libproxy#282 Gbp-Pq: Name tests-Copy-pxProxyFactory-correctly.patch
sir-xw
pushed a commit
to openkylin/libproxy
that referenced
this pull request
Apr 22, 2025
Otherwise there is no guarantee that the object will continue to exist after the original factory is destroyed, and both the copy and the original factory will try to unref it when destroyed, which is a programming error. Bug: libproxy/libproxy#280 Signed-off-by: Simon McVittie <smcv@debian.org> Forwarded: libproxy/libproxy#282 Gbp-Pq: Name px_proxy_factory_copy-Add-a-new-reference-to-the-manager.patch
sir-xw
pushed a commit
to openkylin/libproxy
that referenced
this pull request
Apr 22, 2025
Previously the copy was never freed, which hid the bugs fixed by the previous commits. Bug: libproxy/libproxy#280 Signed-off-by: Simon McVittie <smcv@debian.org> Forwarded: libproxy/libproxy#282 Gbp-Pq: Name tests-Don-t-leak-the-copied-pxProxyFactory.patch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
tests: Copy pxProxyFactory correctly
The second argument to g_boxed_copy() is meant to be a pointer to the
boxed instance itself, not a pointer to a pointer to the boxed instance.
For example,
g_boxed_copy (G_TYPE_STRV, environ)is correct (equivalentto g_strdupv() in this case), but
g_boxed_copy (G_TYPE_STRV, &environ)would be incorrect. The same principle applies here.
The test passed despite this because self->pf and
&self->pf happen to both be pointer-sized, and as a result of
px_proxy_factory_copy doesn't increment refcount of the underlying proxy manager #280, the data that is pointed
to was copied byte-by-byte but not otherwise used; but this will no
longer be true when px_proxy_factory_copy doesn't increment refcount of the underlying proxy manager #280 is fixed, so the test needs fixing first.
px_proxy_factory_copy: Add a new reference to the manager
Otherwise there is no guarantee that the object will continue to exist
after the original factory is destroyed, and both the copy and the
original factory will try to unref it when destroyed, which is a
programming error.
Resolves: px_proxy_factory_copy doesn't increment refcount of the underlying proxy manager #280
tests: Don't leak the copied pxProxyFactory
Previously the copy was never freed, which hid the bugs fixed by the
previous commits.
Reproduces: px_proxy_factory_copy doesn't increment refcount of the underlying proxy manager #280