Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

px_proxy_factory_copy doesn't increment refcount of the underlying proxy manager #280

Closed
smcv opened this issue Feb 27, 2024 · 0 comments · Fixed by #282
Closed

px_proxy_factory_copy doesn't increment refcount of the underlying proxy manager #280

smcv opened this issue Feb 27, 2024 · 0 comments · Fixed by #282

Comments

@smcv
Copy link
Contributor

smcv commented Feb 27, 2024

To reproduce:

  • create a proxy factory
  • duplicate it
  • free both the original and the duplicate

Expected result: no runtime errors

Actual result: Whichever proxy factory is freed first is freed successfully, destroying the underlying proxy manager. Now the other proxy factory has a dangling pointer to a freed proxy manager. When the second proxy factory is freed, GLib will make an attempt to throw a critical warning:

not ok /libproxy/dup - GLib-GObject-FATAL-CRITICAL: g_object_unref: assertion 'G_IS_OBJECT (object)' failed

but this is undefined behaviour, so it might equally well just crash.
smcv added a commit to smcv/libproxy that referenced this issue Feb 27, 2024
@smcv
tests: Copy pxProxyFactory correctly
32e5707 
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>
smcv added a commit to smcv/libproxy that referenced this issue Feb 27, 2024
@smcv
px_proxy_factory_copy: Add a new reference to the manager
218770e 
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>
smcv added a commit to smcv/libproxy that referenced this issue Feb 27, 2024
@smcv
tests: Don't leak the copied pxProxyFactory
e946e65 
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>
@smcv smcv mentioned this issue Feb 27, 2024
Merged
@smcv smcv changed the title px_proxy_factory_copy doesn't duplicate the underlying proxy manager px_proxy_factory_copy doesn't take a new reference to the underlying proxy manager Feb 27, 2024
@smcv smcv changed the title px_proxy_factory_copy doesn't take a new reference to the underlying proxy manager px_proxy_factory_copy doesn't increment refcount of the underlying proxy manager Feb 27, 2024
janbrummer pushed a commit that referenced this issue Apr 2, 2024
@smcv @janbrummer
tests: Copy pxProxyFactory correctly
5cca651 
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
#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>
janbrummer pushed a commit that referenced this issue Apr 2, 2024
@smcv @janbrummer
px_proxy_factory_copy: Add a new reference to the manager
44a6d7e 
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: #280
Signed-off-by: Simon McVittie <smcv@debian.org>
janbrummer pushed a commit that referenced this issue Apr 2, 2024
@smcv @janbrummer
tests: Don't leak the copied pxProxyFactory
4e6202d 
Previously the copy was never freed, which hid the bugs fixed by the
previous commits.

Reproduces: #280
Signed-off-by: Simon McVittie <smcv@debian.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix refcounting in px_proxy_factory_copy (#280) smcv/libproxy
1 participant
@smcv