[Dnsmasq-discuss] [PATCH] Refresh cached socket fd if the interface index changed

[Simon Kelley]

Patch applied, with a few tweaks, mainly for style consistency, but one
to avoid passing strings of length zero to if_nametoindex().


Many thanks.


Cheers,

Simon.



On 25/08/16 16:10, Beniamino Galvani wrote:
> The socket bound to a specific interface in the daemon->sfds cache is
> reused also when the interface disappears and is created again,
> causing resolution problems.
> 
> This problem can be seen when connecting to VPNs with NetworkManager:
> when the VPN is connected NM pushes through D-Bus a configuration
> containing the upstream server '1.2.3.4 at tun0' and dnsmasq creates a
> socket bound to tun0. Later, the VPN is reconnected and tun0 reappears
> with a different ifindex; but even if the server list is updated again
> (still containing an upstream server on tun0), dnsmasq tries to use
> the old socket and any DNS request fails.
> 
> This patch adds a check on the ifindex in allocate_sfd() to prevent
> the reuse of a stale socket, and ensures that unused sockets are
> destroyed.
> ---
>  src/dnsmasq.h |  2 ++
>  src/network.c | 29 +++++++++++++++++++++++++++--
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/src/dnsmasq.h b/src/dnsmasq.h
> index 27385a9..462aaf5 100644
> --- a/src/dnsmasq.h
> +++ b/src/dnsmasq.h
> @@ -487,8 +487,10 @@ union mysockaddr {
>  struct serverfd {
>    int fd;
>    union mysockaddr source_addr;
>    char interface[IF_NAMESIZE+1];
> +  unsigned int ifindex;
> +  unsigned int used;
>    struct serverfd *next;
>  };
>  
>  struct randfd {
> diff --git a/src/network.c b/src/network.c
> index e7722fd..bcb4d1f 100644
> --- a/src/network.c
> +++ b/src/network.c
> @@ -1203,8 +1203,9 @@ int local_bind(int fd, union mysockaddr *addr, char *intname, int is_tcp)
>  
>  static struct serverfd *allocate_sfd(union mysockaddr *addr, char *intname)
>  {
>    struct serverfd *sfd;
> +  unsigned int ifindex = 0;
>    int errsave;
>  
>    /* when using random ports, servers which would otherwise use
>       the INADDR_ANY/port0 socket have sfd set to NULL */
> @@ -1223,14 +1224,19 @@ static struct serverfd *allocate_sfd(union mysockaddr *addr, char *intname)
>  	  addr->in6.sin6_port == htons(0)) 
>  	return NULL;
>  #endif
>      }
> +
> +  if (intname)
> +    ifindex = if_nametoindex(intname);
>        
>    /* may have a suitable one already */
>    for (sfd = daemon->sfds; sfd; sfd = sfd->next )
>      if (sockaddr_isequal(&sfd->source_addr, addr) &&
> -	strcmp(intname, sfd->interface) == 0)
> +	strcmp(intname, sfd->interface) == 0 &&
> +	ifindex == sfd->ifindex) {
>        return sfd;
> +    }
>    
>    /* need to make a new one. */
>    errno = ENOMEM; /* in case malloc fails. */
>    if (!(sfd = whine_malloc(sizeof(struct serverfd))))
> @@ -1249,13 +1255,15 @@ static struct serverfd *allocate_sfd(union mysockaddr *addr, char *intname)
>        free(sfd);
>        errno = errsave;
>        return NULL;
>      }
> -    
> +
>    strcpy(sfd->interface, intname); 
>    sfd->source_addr = *addr;
>    sfd->next = daemon->sfds;
> +  sfd->ifindex = ifindex;
>    daemon->sfds = sfd;
> +
>    return sfd; 
>  }
>  
>  /* create upstream sockets during startup, before root is dropped which may be needed
> @@ -1428,14 +1436,18 @@ void add_update_server(int flags,
>  void check_servers(void)
>  {
>    struct irec *iface;
>    struct server *serv;
> +  struct serverfd *sfd, **ptr;
>    int port = 0, count;
>  
>    /* interface may be new since startup */
>    if (!option_bool(OPT_NOWILD))
>      enumerate_interfaces(0);
>    
> +  for (sfd = daemon->sfds; sfd; sfd = sfd->next)
> +    sfd->used = 0;
> +
>  #ifdef HAVE_DNSSEC
>   /* Disable DNSSEC validation when using server=/domain/.... servers
>      unless there's a configured trust anchor. */
>    for (serv = daemon->servers; serv; serv = serv->next)
> @@ -1504,8 +1516,10 @@ void check_servers(void)
>  			daemon->namebuff, strerror(errno));
>  	      serv->flags |= SERV_MARK;
>  	      continue;
>  	    }
> +
> +	  serv->sfd->used++;
>  	}
>        
>        if (!(serv->flags & SERV_NO_REBIND) && !(serv->flags & SERV_LITERAL_ADDRESS))
>  	{
> @@ -1546,8 +1560,19 @@ void check_servers(void)
>    
>    if (count - 1 > SERVERS_LOGGED)
>      my_syslog(LOG_INFO, _("using %d more nameservers"), count - SERVERS_LOGGED - 1);
>  
> +  /* Remove unused sfds */
> +  for (ptr = &daemon->sfds; *ptr; ) {
> +    sfd = *ptr;
> +    if (!sfd->used) {
> +      *ptr = sfd->next;
> +      close(sfd->fd);
> +      free(sfd);
> +    } else
> +      ptr = &sfd->next;
> +  }
> +
>    cleanup_servers();
>  }
>  
>  /* Return zero if no servers found, in that case we keep polling.
>