X2Go Bug report logs -
#1637
[llm-coauthored] Intermittent SIGSEGV in ssh_select()/ssh_event_add_session() from SshMasterConnection::channelLoop() -- looks like a data race on the channel list
Full log
Message #10 received at 1637@bugs.x2go.org (full text, mbox, reply):
[Message part 1 (text/plain, inline)]
Spend a few more tokens to fix this.
Note that I have not carefully verified the patch, but it seems to fix
both of the segfaults I encountered (but still does not make resume
work, that is a problem on the server side).
I also encountered an infinite loop (100% CPU usage while trying in vain
to start the proxy), but did not generate a patch for that yet, let me
know if you want one.
Cheers,
Philipp
--------------------------------------------------------------------
NOTE ON HOW THIS FOLLOW-UP WAS PRODUCED
--------------------------------------------------------------------
Same as the original report: debugging and patch drafting done
together with an LLM assistant (Claude) at my direction. I built,
ran, and verified everything myself. Flagging this again per the
original subject tag.
--------------------------------------------------------------------
SUMMARY
--------------------------------------------------------------------
Root cause found and fixed. The crash was NOT a libssh bug or a
generic race -- it was a x2goclient-side logic bug in
SshMasterConnection::createChannelConnection() / channelLoop(),
specifically a dangling-pointer bug in the array x2goclient hands to
ssh_select(). Two distinct issues in that area, both patched below.
I've been running the patched build since and no longer see this
crash, including in a case that previously crashed within seconds of
"resuming normal session".
--------------------------------------------------------------------
ROOT CAUSE #1: stale channel pointer not cleared on error
--------------------------------------------------------------------
In createChannelConnection() (src/sshmasterconnection.cpp), when
ssh_channel_open_session() or ssh_channel_request_exec() fails after
a channel has already been stored via
channelConnections[i].channel = channel;
the error paths do:
ssh_channel_free (channel);
...
return (false);
without resetting channelConnections[i].channel back to 0. The
caller in channelLoop() just does "continue;" on failure, so the
stale entry survives.
On the *next* pass, createChannelConnection() checks:
if ( channelConnections.at ( i ).channel==0l )
which is false (the pointer is non-null, just freed), so the
"create a new channel" branch is skipped entirely, and the dangling
pointer is handed straight to ssh_select() via read_chan[i]. Confirmed
with valgrind: an ssh_channel allocated and freed with both call
sites inside channelLoop() itself, then read again on the very next
ssh_select() pass.
Fix: explicitly reset channelConnections[i].channel = 0l on both
error paths, right after ssh_channel_free().
--------------------------------------------------------------------
ROOT CAUSE #2: uninitialized array slot on same-iteration failure
--------------------------------------------------------------------
Separately, in channelLoop():
ssh_channel* read_chan=new ssh_channel[channelConnections.size() +1];
ssh_channel* out_chan=new ssh_channel[channelConnections.size() +1];
read_chan[channelConnections.size() ]=NULL;
new T[n] does not zero-initialize a plain pointer array. For any
index whose createChannelConnection() call fails and returns early
(before reaching the "read_chan[i] = ..." line at the end of that
function), read_chan[i] is left as raw heap garbage, not NULL --
so ssh_select()'s NULL-terminated scan of the array walks straight
into it. This one can crash within a single channelLoop() iteration,
with no prior history needed, which is what I saw when a channel
failed immediately on session resume.
Fix: value-initialize both arrays ("new ssh_channel[n]()") so every
untouched slot starts NULL rather than garbage.
--------------------------------------------------------------------
PATCH
--------------------------------------------------------------------
Both fixes, against the 4.1.2.3 source tree (attached in full as
x2goclient-dangling-channel-v2.patch; apply with "patch -p0" from
the top of the source tree):
--- src/sshmasterconnection.cpp.orig
+++ src/sshmasterconnection.cpp
@@ -2205,6 +2205,11 @@
/* Free channel. */
ssh_channel_free (channel);
+ /* Local patch: without this, channelConnections[i].channel
+ * keeps pointing at freed memory, and the next loop
+ * iteration's "channel==0l" check wrongly treats it as
+ * still valid, handing a dangling pointer straight to
+ * ssh_select(). */
+ channelConnections[i].channel = 0l;
emit ioErr ( channelConnections[i].creator, errorMsg,
err );
x2goDebug<<errorMsg.left (errorMsg.size () - 1)<<":
"<<err<<endl;
@@ -2219,6 +2224,8 @@
/* Close connection and free channel. */
ssh_channel_close (channel);
ssh_channel_free (channel);
+ /* Local patch: see comment above. */
+ channelConnections[i].channel = 0l;
emit ioErr ( channelConnections[i].creator, errorMsg,
err );
x2goDebug<<errorMsg.left (errorMsg.size () - 1)<<":
"<<err<<endl;
@@ -1959,8 +1966,14 @@
}
- ssh_channel* read_chan=new
ssh_channel[channelConnections.size() +1];
- ssh_channel* out_chan=new ssh_channel[channelConnections.size()
+1];
+ /* Local patch: value-initialize ("()") so every slot starts NULL.
+ * createChannelConnection() can return early (on failure) before
+ * it reaches the line that fills in read_chan[i]/out_chan[i];
+ * without zero-init that slot is uninitialized garbage, not
+ * NULL, and ssh_select() walks off the end of it looking for a
+ * NULL terminator -- causing exactly the SIGSEGV in ssh_select()
+ * this works around. */
+ ssh_channel* read_chan=new
ssh_channel[channelConnections.size() +1]();
+ ssh_channel* out_chan=new ssh_channel[channelConnections.size()
+1]();
read_chan[channelConnections.size() ]=NULL;
(Line numbers are approximate/context-shifted between the two hunks
above; the attached patch file has exact, applicable hunks.)
--------------------------------------------------------------------
CAVEAT / OPEN QUESTION FOR MAINTAINERS
--------------------------------------------------------------------
My fix for issue #2 (zero-init) stops the crash but isn't fully
correct: ssh_select() stops at the first NULL it finds in the
channels[] array. If channel i fails and is left NULL while later
channels i+1, i+2, ... succeeded, this round's ssh_select() call
silently only covers channels 0..i-1 -- the rest are skipped for
that ~500ms iteration (self-corrects next iteration, since the array
is rebuilt fresh each pass, but it's not truly "correct"). A proper
fix would compact the array to skip failed slots rather than
truncate at them. I kept my patch minimal/local rather than
attempting that restructuring myself.
--------------------------------------------------------------------
NEXT BUG: INFINITE LOOP ON CONNECTION FAILURE
--------------------------------------------------------------------
Separately, while testing the fix above under a failure condition (the
server-side issue described below), I caught a related but distinct
problem in the same function: when ssh_channel_open_session() keeps
failing (e.g. the underlying SSH transport is still "connected" but the
server keeps rejecting the channel), channelLoop() has no failure cap or
backoff -- it just retries immediately, forever. With the crash fixed,
this now manifests as a thread spinning at 100% CPU indefinitely, doing
real cryptographic work each iteration (confirmed via gdb: the hot
thread was consistently inside ssh_channel_open_session(), not an empty
spin), with no user-visible error and no give-up condition. I haven't
attempted a patch for this one since it's more of a design/policy gap
(how many retries, what backoff, how to surface the eventual failure to
the user) than a straightforward bug fix, and I'd rather get a read on
whether the two patches above are wanted before writing more.
--------------------------------------------------------------------
UNRELATED FINDING WHILE DEBUGGING (separate report, x2goserver not
x2goclient)
--------------------------------------------------------------------
While chasing why sessions still failed to connect after this fix,
I found a real, reproducible bug on the server side, in x2goserver
(not x2goclient) -- filing this separately, but noting it here since
it's what made the client-side race easy to trigger in the first
place (rapid failed reconnect/channel-creation attempts):
/usr/lib/x2go/x2gormport line 34:
my $port=shift or die;
"shift or die" is falsy-checked, not definedness-checked, so this
(and two equivalent lines further down the call chain, in
X2Go::Server::DB::db_rmport and the SQLite3 backend's db_rmport)
dies whenever a legitimately-zero port value is passed -- which
happens routinely for ports not previously in use by a session.
Reproduced directly server-side:
$ x2goresume-session pklenze-58-... 1920x1080 adsl 16m-jpeg-9 us
pc105/us 1 both no
Died at /usr/lib/x2go/x2gormport line 34.
Died at /usr/lib/x2go/x2gormport line 34.
gr_port=34759
sound_port=34760
fs_port=34761
Connection to lxi111 closed by remote host.
The resume script prints the port numbers regardless of whether
x2gormport actually succeeded, so the client is told to forward to
a port whose server-side setup silently failed -- producing
"Channel opening failure ... Connection refused" client-side, which
is what started this whole thread. I'll file this against x2goserver
separately once I've confirmed the fix; flagging here for context /
in case it's useful to anyone hitting a similar "resume looks fine
but graphics never connects" symptom.
--------------------------------------------------------------------
ATTACHMENT
--------------------------------------------------------------------
x2goclient-dangling-channel-v2.patch -- full patch, apply with
"patch -p0" from the top of the x2goclient-4.1.2.3 source tree.
[x2goclient-dangling-channel-v2.patch (text/x-patch, attachment)]
Send a report that this bug log contains spam.
X2Go Developers <owner@bugs.x2go.org>.
Last modified:
Tue Sep 29 16:42:51 2026;
Machine Name:
ymir.das-netzwerkteam.de
X2Go Bug tracking system
Debbugs is free software and licensed under the terms of the GNU
Public License version 2. The current version can be obtained
from https://bugs.debian.org/debbugs-source/.
Copyright © 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson,
2005-2017 Don Armstrong, and many other contributors.