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.

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

OTHER: fix c++ client dangling reference #5

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Dec 6, 2018

On #xmm2 Chewi reported c++/tut7 to be broken at start:

GLib-WARNING **: glib-2.56.2/glib/giounix.c:410
Error while getting flags for FD: Bad file descriptor (9)

valgrind shows the problem as read of uninitialized data:

$ valgrind ./tut7

==32268== Conditional jump or move depends on uninitialised value(s)
==32268==    at 0x49DC36B: xmmsc_mainloop_gmain_init (xmmsclient-glib.c:80)
==32268==    by 0x49E11BE: Xmms::GMainloop::GMainloop(xmmsc_connection_St*) (xmmsclient++-glib.cpp:11)
==32268==    by 0x10C64D: main (in /home/slyfox/dev/git/xmms2-devel/doc/tutorial/c++/tut7)
==32268==  Uninitialised value was created by a stack allocation
==32268==    at 0x49E119A: Xmms::GMainloop::GMainloop(xmmsc_connection_St*) (xmmsclient++-glib.cpp:8)

This happens due to use of dangling C++ reference to stack variable:

// somewhere in src/include/xmmsclient/xmmsclient++/mainloop.h
class MainloopInterface {
    MainloopInterface( xmmsc_connection_t* conn ) :
        running_( false ), conn_( conn ) { }
  protected:
    bool running_;
    xmmsc_connection_t*& conn_;
}

Note: conn_ refers to dangling local variable of
MainloopInterface::MainloopInterface constructor.

The fix is to pass through pointer reference.
MainLoop::MainLoop() already does it.

Reported-by: James Le Cuirot
Signed-off-by: Sergei Trofimovich slyfox@gentoo.org

On #xmm2 Chewi reported c++/tut7 to be broken at start:

```
GLib-WARNING **: glib-2.56.2/glib/giounix.c:410
Error while getting flags for FD: Bad file descriptor (9)
```

valgrind shows the problem as read of uninitialized data:

```
$ valgrind ./tut7

==32268== Conditional jump or move depends on uninitialised value(s)
==32268==    at 0x49DC36B: xmmsc_mainloop_gmain_init (xmmsclient-glib.c:80)
==32268==    by 0x49E11BE: Xmms::GMainloop::GMainloop(xmmsc_connection_St*) (xmmsclient++-glib.cpp:11)
==32268==    by 0x10C64D: main (in /home/slyfox/dev/git/xmms2-devel/doc/tutorial/c++/tut7)
==32268==  Uninitialised value was created by a stack allocation
==32268==    at 0x49E119A: Xmms::GMainloop::GMainloop(xmmsc_connection_St*) (xmmsclient++-glib.cpp:8)
```

This happens due to use of dangling C++ reference to stack variable:

```
// somewhere in src/include/xmmsclient/xmmsclient++/mainloop.h
class MainloopInterface {
    MainloopInterface( xmmsc_connection_t* conn ) :
        running_( false ), conn_( conn ) { }
  protected:
    bool running_;
    xmmsc_connection_t*& conn_;
}
```

Note: `conn_` refers to dangling local variable of
`MainloopInterface::MainloopInterface` constructor.

The fix is to pass through pointer reference.
`MainLoop::MainLoop()` already does it.

Reported-by: James Le Cuirot
Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
@zharf
Copy link
Contributor

zharf commented Dec 6, 2018

I'm actually a little too tired to think right now but at least GMainloop constructor also needs to have this change as well...

gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Dec 6, 2018
Picked patch from xmms2/xmms2-devel#5

Reported-by: James Le Cuirot
Package-Manager: Portage-2.3.52, Repoman-2.3.12
Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
@trofi
Copy link
Contributor Author

trofi commented Dec 6, 2018

Good catch! That xmmc_connection_t *& API is really hard to use right :) I'll see if client can switch it to xmmc_connection_t * and not allow the pointer to change underneath.

@trofi trofi marked this pull request as draft January 1, 2023 21:59
@trofi
Copy link
Contributor Author

trofi commented Jan 1, 2023

Converting to draft as I think it introduces dangling reference to a temporary on initial object creation. I'll spend some time tracing through lifetimes more explicitly and redo.

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