channels.c:3258]: (warning) The comparison operator in 'strcmp(listen_addr,"localhost") != 0' should maybe be '==' instead, currently the expression 'strcmp(listen_addr,"127.0.0.1") == 0' is redundant. Source code is } else if (strcmp(listen_addr, "localhost") != 0 || strcmp(listen_addr, "127.0.0.1") == 0 || strcmp(listen_addr, "::1") == 0) { /* Accept localhost address when GatewayPorts=yes */ addr = listen_addr; } Maybe better code } else if (strcmp(listen_addr, "localhost") == 0 || strcmp(listen_addr, "127.0.0.1") == 0 || strcmp(listen_addr, "::1") == 0) { /* Accept localhost address when GatewayPorts=yes */ addr = listen_addr; }
AFAIK this is actually correct and support the case of "localhost" requesting both IPv4 and IPv6 loopback addresses as well as GatewayPorts=clientspecified First, this code is only reachable for GatewayPorts=clientspecified or we're on the client side (where the client gets to specify the listen address always). Here we want to allow arbitrary listen addresses, but "localhost" needs special-casing because it should yield listeners that respond to both IPv4 and IPv6 connections. So: 1) if listen_addr=="::1" or "127.0.0.1" we accept it as addr 2) If listen_addr=="localhost" we leave addr=NULL and *wildcardp is set to 0. 3) If listen_addr is anything else, then we accept it as addr. Later, addr/wildcard is used like this: 3294 /* Determine the bind address, cf. channel_fwd_bind_addr() comment */ 3295 addr = channel_fwd_bind_addr(fwd->listen_host, &wildcard, 3296 is_client, fwd_opts); 3297 debug3("%s: type %d wildcard %d addr %s", __func__, 3298 type, wildcard, (addr == NULL) ? "NULL" : addr); 3299 3300 /* 3301 * getaddrinfo returns a loopback address if the hostname is 3302 * set to NULL and hints.ai_flags is not AI_PASSIVE 3303 */ 3304 memset(&hints, 0, sizeof(hints)); 3305 hints.ai_family = ssh->chanctxt->IPv4or6; 3306 hints.ai_flags = wildcard ? AI_PASSIVE : 0; 3307 hints.ai_socktype = SOCK_STREAM; 3308 snprintf(strport, sizeof strport, "%d", fwd->listen_port); 3309 if ((r = getaddrinfo(addr, strport, &hints, &aitop)) != 0) { so listen_addr == localhost yields getaddrinfo(NULL,...) that give us sockets for both IPv4 and IPv6 (yes, this is confusing)
(In reply to Damien Miller from comment #1) > AFAIK this is actually correct Righto. > (yes, this is confusing) So it could do with simplifying. The code I mentioned does this kind of thing: else if (x != A || x == B || x == C) That can be simplified downto else if (x != A) with no loss of functionality.
Close all resolved bugs after release of OpenSSH 7.7.
Damien, could we simplify this code as explained in the comment #2 and possibly add a comment so it is clear for future readers? This popped up in results of our static analyzers scanner as a suspicious code again. https://github.com/openssh/openssh-portable/blob/master/channels.c#L3356
The special cases are already documented in the comment above the function, but I'll add another comment.
close bugs that were resolved in OpenSSH 8.5 release cycle