Fix for int16 overflow bug causing connection to get re-routed to UNIX socket

[Avinodh]

The issue is described in Issue #746. Copying the description here:

Description:
The feature introduced in v-1.17 which enables pgbouncer to round-robin over a comma-separated host list has a bug, which results in connections to a client defined with a host list being re-routed to the UNIX socket path instead (/tmp). Details and repro below:

In the following block of code responsible for parsing the host-list and round-robining across the hosts:

    /* host list? */
    if (db->host && strchr(db->host, ',')) {
	int count = 1;
	int n;
	for (const char *p = db->host; *p; p++)
		if (*p == ',')
			count++;

	host_copy = xstrdup(db->host);
         for (host = strtok(host_copy, ","), n = 0; host; host = strtok(NULL, ","), n++)
                if (server->pool->rrcounter % count == n)
                    break;
          Assert(host);

          server->pool->rrcounter++;
    } else {
        host = db->host;
    }

    if (!host || host[0] == '/' || host[0] == '@') {
        const char *unix_dir;
       . . .

rrcounter is of type int16_t (declaration). This counter is never reset, as a result of which it eventually overflows after 32767 connections.
In the code above, once we overflow, rrcounter effectively becomes negative. At this point, the condition:
server->pool->rrcounter % count == n will always evaluate to false except for when count == 1.

To understand the issue that follows, consider the following pgbouncer.ini config:

[databases]
database = host=endpoint1 port=5432 dbname=database pool_mode=transaction 
database-rr = host=endpoint-rr-1,endpoint-rr-2,endpoint-rr-3,endpoint-rr-4,endpoint-rr-5,endpoint-rr-6 dbname=database pool_mode=transaction 

After 32767 connections are made to the database-rr client, rrcounter will overflow resulting in a negative value. Any following connections to database-rr will result in the conditional noted above to evaluate to false, resulting in the for loop to complete leaving host == nullptr. The assertion Assert(host) will not fire (assuming you don't compile with --enable-cassert).

Now that host == nullptr, the subsequent conditional if (!host || host[0] == '/' || host[0] == '@') will evaulate to true, thereby re-routing the connection to the UNIX socket.

In the above example, it just so happens that the dbname == database for the database-rr client matches the client name database. Therefore, when re-routed to the UNIX socket, it will end up getting forwarded to the database client.

Assuming we had database-rr as the only client defined, the re-routed connection would fail to connect.

Fix:
The fix here is fairly straight-forward- to change int16_t rrcounter; to uint16_t rrcounter; which will prevent overflowing to negative.

[JelteF]

Thank you for your contribution and detailed explanation of the fix :)