Subject: "connectedHost" variable contains wrong IP, reason unknown (was: Re: [X2Go-User] Pb with x2go agent on ppc64 system)

From: Stefan Baur <[email-masked]>
Date: Wed, 11 Jun 2014 23:46:08 +0200

Package: x2goagent
severity: normal
version: 3.5.0.24

Note: Crossposting and Reply-To'ing to X2Go-Dev due to the need for a
C/C++ coder, also, turning it into a bug so we can keep track of this.

Here's the output the user gets as the connection refuses to establish
itself:

quoting Sebastien Chabrolles:

> running as X2Go Agent
> 
> NXAGENT - Version 3.5.0
> 
> Copyright (C) 2001, 2011 NoMachine. See http://www.nomachine.com/
> for more information.
> 
> Info: Agent running with pid '24808'. Session: Starting session at
> 'Sat Jun  7 15:37:37 2014'. Info: Proxy running in server mode with
> pid '24808'. Info: Waiting for connection from 'localhost' on port
> '30003'. Warning: Refusing connection from '225.3.10.80'. .80' on
> port '30003'. I don't know what is this IP address !!!!!

And at a later attempt, Sebastien received this output:
> running as X2Go Agent
> 
> 
> NXAGENT - Version 3.5.0
> 
> 
> Copyright (C) 2001, 2011 NoMachine.
>  See http://www.nomachine.com/ for more information.
> 
> 
> Info: Agent running with pid '10710'.
>  Session: Starting session at 'Wed Jun 11 23:02:09 2014'.
>  Info: Proxy running in server mode with pid '10710'.
>  Info: Waiting for connection from 'localhost' on port '30006'.
>  Warning: Refusing connection from '141.168.10.80'.
>  Info: Aborting the procedure due to signal '1'.
>  Error: Aborting session with 'Unable to open display
> 'nx/nx,options=/root/.x2go/C-root-51-1402520526_stRWWWBROWSER_dp24/options:51''.
>  Session: Aborting session at 'Wed Jun 11 23:02:19 2014'.
>  Session: Session aborted at 'Wed Jun 11 23:02:19 2014'.

My first guess is/was that there's something endianess-related going
wrong, as he's running X2Go on a ppc64 architecture instead of x86/x64.
However, the fact that the two "wrong" IPs change at random speak
against that, so I may be totally wrong with this and there's a much
simpler reason and solution.

quoting myself, replying to Mike Gabriel there:

>> Do you think you could at least give him a hint as to where that 
>> message is pulling the IP from and how it is being
>> mangled/processed before that?
>> 
>> My guess is that there's some self-made conversion routine
>> somewhere that only works on little-endian architectures like x86
>> and x64.
>> 
>> PPC64 is big-endian by default.

quoting Mike Gabriel:

> you can get the nx-libs sources from git.x2go.org [1] and grep
> through the sources.
> 
> I am currently doing the same to get NX fixed on systems with 
> poly-instantiated /tmp directories.

Here's what I was able to find out so far:

I was able to locate the message in Loop.cpp.
It uses the variable "connectedHost".
So "connectedHost" contains the wrong IP.

connectedHost gets populated (also in Loop.cpp) like this:

char *connectedHost = inet_ntoa(newAddr.sin_addr);

so either newAddr.sin_addr already contains a wrong value (I'm not sure
how to check that, though), or inet_ntoa does something wrong, or both.

I haven't touched C/C++ code since the year 2002 or so, so debugging
this further is way beyond my ken.

Any one of the more experienced coders willing to jump in?

Here's some more info about the X2Go server system as provided by
Sebastien Chabrolles:

# cat /etc/hosts
127.0.0.1   localhost localhost.localdomain localhost4
localhost4.localdomain4
::1         localhost localhost.localdomain localhost6
localhost6.localdomain6
10.7.19.161     fc20-161

# cat /etc/resolv.conf
nameserver 129.35.160.4

#cat /etc/nsswitch.conf
passwd:     files
shadow:     files
group:      files
###
#this strange-looking entry was present during the first tries.
#
#hosts:      files mdns4_minimal [NOTFOUND=return] dns myhostname
#
# it was changed to the one below for the last try, which gave
# the result with the different, but still wrong, IP.
###
hosts:      files dns
bootparams: nisplus [NOTFOUND=return] files
ethers:     files
netmasks:   files
networks:   files
protocols:  files
rpc:        files
services:   files
netgroup:   files
publickey:  nisplus
automount:  files
aliases:    files nisplus

# ifconfig -a
eth0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
        inet 10.7.19.161  netmask 255.255.255.0  broadcast 10.7.19.255

        inet6 fe80::f816:3eff:feec:ecb8  prefixlen 64  scopeid
0x20<link>
        ether fa:16:3e:ec:ec:b8  txqueuelen 1000  (Ethernet)

        RX packets 90677  bytes 5717673 (5.4 MiB)

        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 89102  bytes 32828059 (31.3 MiB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

lo: flags=73<UP,LOOPBACK,RUNNING>  mtu 65536
        inet 127.0.0.1  netmask 255.0.0.0
        inet6 ::1  prefixlen 128  scopeid 0x10<host>
        loop  txqueuelen 0  (Local Loopback)
        RX packets 76  bytes 5248 (5.1 KiB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 76  bytes 5248 (5.1 KiB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

# route -n
Kernel IP routing table
Destination     Gateway         Genmask         Flags Metric Ref
Use Iface
0.0.0.0         10.7.19.254     0.0.0.0         UG    0      0
0 eth0
10.7.19.0       0.0.0.0         255.255.255.0   U     0      0
0 eth0
169.254.0.0     0.0.0.0         255.255.0.0     U     1002   0
0 eth0

- -Stefan

---

From: Stefan Baur <[email-masked]>
Date: Thu, 12 Jun 2014 21:59:59 +0200

Guys, please cc: [email-masked] instead of
[email-masked] - no idea how that address ended up in here.
(CCing so it shows up in the bugtracker)

Am 12.06.2014 21:48, schrieb Mihai Moldovan:
> Hello Sebastien
> 
> Please don't mind me answering inline.
> 
> 
> * On 12.06.2014 07:28 pm, sebastien chabrolles wrote:
>> So, I don't say that the problem is due to Endianness, but we have to keep in
>> mind that fedora 20 ppc64 is BIG Endian.
> 
> Yes, but big endian is fine. All those functions operate in network byte order,
> which is big endian. No conversion is taking place in the code, as that would be
> unnecessary anyway. Only inet_ntoa() is writing the quad dotted decimal address
> to a string buffer in host endianness. However, this is done by the operating
> system, so NX can (and is) agnostic about it.
> 
> 
>> For the connection, I use IPV4 addresse, no hostname. There is only one IP on
>> the server.
> 
> OK. I was asking because the code is calling gethostbyname() which, when given a
> host name, looks up the address of that specified host name. If, however, the
> host name should have multiple addresses, all of them will be returned in a list
> -- their order depending on how the DNS server spits them out. Most DNS servers
> use a round-robin method for this, so you will get a different ordering with
> each lookup.
> 
> That was one possible explanation for what you might have experienced.
> 
> However, if you're only using IP addresses directly everywhere, gethostbyname()
> merely converts the given string to its binary address representation in network
> byte order. Thus, the functionality is almost equal to inet_aton() (just the way
> the information is returned differs, but that's no functional change.)
> 
> 
>> If you need to have access to the server, this can be done with openVPN
>> certificate. This is only a "sandbox server" to reproduce the x2go issue.
> 
> Providing a means to debug the issue would be great, because at that time I have
> no valid explanation for what is happening there.
> 
> Were those packages installed via binary packages? Unfortunately, x2go doesn't
> ship debug builds.
> 
> Please make sure that the machine in question features build tools and gdb, as
> nxcomp/nxproxy will need to be compiled with debug information in order to get
> anything interesting out of it.

---

From: sebastien chabrolles <[email-masked]>
Date: Fri, 13 Jun 2014 18:10:32 +0200

Hi Guys,

Just to tell you, I have reproduce the issue on another machine in RedHat 
7.0 ppc64

IP of this system is different (10.7.19.168) which could explain the 
different IP in the session.log...
But again, I don't know what is this IP addresse. ....

running as X2Go Agent

NXAGENT - Version 3.5.0

Info: Agent running with pid '11574'.
Session: Starting session at 'Fri Jun 13 17:00:02 2014'.
Info: Proxy running in server mode with pid '11574'.
Info: Waiting for connection from 'localhost' on port '30003'.
Warning: Refusing connection from '138.119.178.116'.
116' on port '30003'.

Cordialement / Best Regards

Sébastien Chabrolles
Power Systems Benchmark Specialist
IBM Client Center, Montpellier 
1 rue vieille poste 34000 Montpellier FRANCE 
Tel +33 4 67 34 40 95
Email : [email-masked]

Sauf indication contraire ci-dessus:/ Unless stated otherwise above:
Compagnie IBM France
Siège Social : 17 avenue de l'Europe, 92275 Bois-Colombes Cedex
RCS Nanterre 552 118 465
Forme Sociale : S.A.S.
Capital Social : 655.732.332,20 ?
SIREN/SIRET : 552 118 465 03644 - Code NAF 6202A

---

From: Mihai Moldovan <[email-masked]>
Date: Fri, 13 Jun 2014 18:15:27 +0200

> the information is returned differs, but that's no functional change.)

Follow-up: I've written a simple program to look up hosts using the OS-provided
functions getaddrinfo() and inet_ntop(). If you (Sebastien) are interested, take
a look in /usr/src/domainlookup, although it's really boring.

Output for "./getaddrinfo localhost 127.0.0.1 loopback":

### information for localhost ###
addr 1: 127.0.0.1 (length 9)
addr 2: 127.0.0.1 (length 9)
addr 3: 127.0.0.1 (length 9)
addr 4: 127.0.0.1 (length 9)
addr 5: 127.0.0.1 (length 9)
addr 6: 127.0.0.1 (length 9)

### information for 127.0.0.1 ###
addr 1: 127.0.0.1 (length 9)
addr 2: 127.0.0.1 (length 9)
addr 3: 127.0.0.1 (length 9)

### information for loopback ###

Thus, it doesn't look like a system issue. The returned addresses are just fine.
Even if something WERE to convert 127.0.0.1 to "localhost" and back to an
address, the address list for localhost is fine and does not contain the unknown
addresses.

Seems like we're really seeing an nx bug on ppc64.

I'll supply more information later when I get back.

Mihai

---

From: Mihai Moldovan <[email-masked]>
Date: Sat, 14 Jun 2014 03:05:02 +0200

* On 14.06.2014 01:22 am, Mihai Moldovan wrote:
> WaitForRemote(int) is being called by nxagent/x2goagent. Thus, I should be
> rather debugging that and not nxproxy...

Sorry for sending the last eMail to the wrong bug number, Mike. I got confused
with the threads and was adding the addressing manually. :(

However, I have good news for #515. I was able to fix the bug.

I'll be writing a roundup and attaching a patch, soon.

---

From: Mihai Moldovan <[email-masked]>
Date: Sat, 14 Jun 2014 03:40:07 +0200

Basically, the addresses Sebastien has seen were "nothing more" than
uninitialized memory.

The underlaying issue was that accept() wants a socklen_t *addrlen parameter.
The original code created a size_t variable to store the size of the sockaddr
structure where information like the IP address is to be written. This value has
been, correctly, set to 16. It was then casted to (socklen_t) and given to accept().

The problem, however, is that socklen_t is defined as a 4 byte integer, while
size_t is at least 8 bytes wide on ppc64. (I highly recommend reading man 2
accept and the in the man page included Linus Torvald rant on that matter!)

This is "not a big deal" on little endian machines, as casting size_t to
socklen_t there will only cut the upper 4 bytes off and leave you with the lower
4 bytes -- which is just fine, as struct sockaddr isn't bigger than 2^32-1 bytes
anyway. 16 stays 16 in that case.

Doing the same thing on big endian machines, however, cuts the lower 4 bytes off
and leaves you with the upper 4 bytes. As the original value was "16", the upper
4 bytes were all zero.

Thus, zero was passed as the addrlen parameter to accept().

As the addrlen parameter defines how many bytes may be written to the sockaddr
structure given to accept(), the function didn't even touch addr. Hence, addr
was left in its original, un-initialized state.

It was an endianness issue after all, but not exactly where anyone would have
expected it.

The fix is trivial -- don't use size_t as the type of the addrlen variable, but
socklen_t, which will also remove the need of casting.

Mike: I hope the patch applies fine. I have quiltimported all other patches
before, so my patch is based on the patched nx-libs repository.
diff --git a/nxcomp/Loop.cpp b/nxcomp/Loop.cpp
index c9617c3..955be85 100644
--- a/nxcomp/Loop.cpp
+++ b/nxcomp/Loop.cpp
@@ -6854,9 +6854,9 @@ int WaitForRemote(int portNum)
     {
       sockaddr_in newAddr;
 
-      size_t addrLen = sizeof(sockaddr_in);
+      socklen_t addrLen = sizeof(sockaddr_in);
 
-      newFD = accept(proxyFD, (sockaddr *) &newAddr, (socklen_t *) &addrLen);
+      newFD = accept(proxyFD, (sockaddr *) &newAddr, &addrLen);
 
       if (newFD == -1)
       {

---

From: Mihai Moldovan <[email-masked]>
Date: Sat, 14 Jun 2014 06:26:24 +0200

Oh, and thanks again to Sebastien for providing access to a Power8 machine for
debugging and building purposes!

---

From: Stefan Baur <[email-masked]>
Date: Sat, 14 Jun 2014 11:19:47 +0200

> It was an endianness issue after all, but not exactly where anyone would have

Ha! :-D

@Mihai: Thanks for helping Sebastien out.

@Sebastien: Once your customer's appliance is ready, feel free to add a
description of it at
http://wiki.x2go.org/doku.php/doc:deployment-stories:start (you will
need a Wiki account for this). We're always happy to list success
stories from our users. :-)

-Stefan