GNOME Bugzilla – Bug 754194
vino crashes on connect when resolver on the server is not using DNS
Last modified: 2015-08-28 16:06:49 UTC
Created attachment 310113 [details] [review] patch for the problem When a server does not have DNS configured and relies on hosts file for resolution, getnameinfo() in server/libvncserver/rfbserver.c(169) fails and leaves the variable "host" uninitialized. Depending on your luck, this can either lead to garbage characters displayed in the notification bubble (and logged in ~/.xsession-errors) to vino crashing and terminating the session, to locking up and preventing the next copy to start. The issue observed on Centos 6.7, vino 2.28.1, most recent vino version has the same problem. Patch and a few screenshots attached.
Created attachment 310114 [details] garbage in notification
Created attachment 310115 [details] .xsession-errors fragment
Created attachment 310116 [details] code fragment
Created attachment 310140 [details] [review] improved patch Thanks for the bug report and patch! Upstream libvncserver also logs an error message, and does this slightly differently, by checking the return code of getnameinfo(). Does this patch work for you?
David, Can I suggest something like the attached patch? Not having DNS is technically not an error, but an unusual configuration. Even when the resolver is not using the DNS, we still have the IP/IPV6 address we can decode and show. The attached patch is doing just that using inet_ntop(). Thanks for your prompt response!
Created attachment 310185 [details] [review] patch with inet_ntop() fallback
Review of attachment 310185 [details] [review]: ::: vino-2.28.1/server/libvncserver/rfbserver.c.m1 @@ +178,3 @@ + if (!gotHostName) { + struct sockaddr_in *addr_in = (struct sockaddr_in *)&addr; + inet_ntop(AF_INET, &(addr_in->sin_addr), host, INET_ADDRSTRLEN); Can you call getnameinfo() with NI_NAMEREQD, and then repeat the call with NI_NUMERICHOST if it fails? Does that avoid the need to add the net_ntop() calls?
The way I understand the getnameinfo() documentation, is that when you don't specify any flags, that's exactly what it tries to do, get the name first, when it fails, return numeric: NI_NUMERICHOST If set, then the numeric form of the hostname is returned. (When not set, this will still happen in case the node's name cannot be determined.) That's in theory. In practice it just fails. I don't know. Maybe it's worth checking out how getnameinfo() is implemented.
(In reply to Dimitri Tarassenko from comment #8) > The way I understand the getnameinfo() documentation, is that when you don't > specify any flags, that's exactly what it tries to do, get the name first, > when it fails, return numeric: > > NI_NUMERICHOST > If set, then the numeric form of the hostname is returned. (When not set, > this will still happen in case the node's name cannot be determined.) > > That's in theory. In practice it just fails. I don't know. Maybe it's worth > checking out how getnameinfo() is implemented. Does getnameinfo() fail to return the numeric form even when passing NI_NUMERICHOST? That would be unexpected. Does the error code returned by getnameinfo() suggest anything useful? he current implementation of getnameinfo() seems to be: http://sourceware.org/git/?p=glibc.git;a=blob;f=inet/getnameinfo.c;h=0126f2032655e5bb4d610c9eddf22ae10ee54cf3;hb=HEAD#l173 I haven't had time to understand it completely, but it seems that the fallback behaviour (which uses inet_ntop()) should be triggered.
I ran it under debugging - getnameinfo() returns -3 (EAI_AGAIN), which is returned from gethostbyaddr() (http://code.woboq.org/userspace/glibc/inet/getnameinfo.c.html line 272. Which is why it doesn't try to return the numeric hostname.
I.e. your suggestion WOULD work, because with NI_NUMERICHOST set it bypasses this attempt and goes directly to using inet_ntop() calls. There is some extra magic happening with IPV6 scopes, so doing what you suggested would be preferred to using inet_ntop() directly as in my patch. Let me try your suggestion today.
Actually, worked great. Patch attached.
Created attachment 310204 [details] [review] patch with second call to getnameinfo() with NI_NUMERICHOST
Review of attachment 310204 [details] [review]: Thanks for the testing! I pushed a slightly-modified version of your patch, with a commit message, to master.