fix get port from free_list of port pool

[button-chen]

fix get port from free_list of port pool

Thanks

[rfuchs]

Can you explain which problem this fixes? Because it's not obvious to me

[button-chen]

Can you explain which problem this fixes? Because it's not obvious to me

static void release_port_now(socket_t *r, struct intf_spec *spec) {
	unsigned int port = r->local.port;
	struct port_pool *pp = &spec->port_pool;

	__C_DBG("trying to release port %u", port);

	if (close_socket(r) == 0) {
		__C_DBG("port %u is released", port);
		iptables_del_rule(r);
		bit_array_clear(pp->ports_used, port);
		g_atomic_int_inc(&pp->free_ports);
		if ((port & 1) == 0) {
			mutex_lock(&pp->free_list_lock);
			if (!bit_array_isset(pp->free_list_used, port)) {
				g_queue_push_tail(&pp->free_list, GUINT_TO_POINTER(port));
				bit_array_set(pp->free_list_used, port);
			}
			mutex_unlock(&pp->free_list_lock);
		}
	} else {
		__C_DBG("port %u is NOT released", port);
	}
}

For example, if the release_port_now function is called, assuming that the currently released port is A, then the bit array ports_used clears the flag indicating that port A is being used, which is port A
It is also added to the list free_list and the free_list_used flag is set. So up to now, port A can be used in the bit array ports_used, which is also considered
Is the free port in free_list. So let's move on

int __get_consecutive_ports(GQueue *out, unsigned int num_ports, unsigned int wanted_start_port,
		struct intf_spec *spec, const str *label)
{
		......
		port += PORT_RANDOM_MIN + (ssl_random() % (PORT_RANDOM_MAX - PORT_RANDOM_MIN));
		__C_DBG("after  randomization port=%d", port);

		// debug msg if port is in the given interval
		if (bit_array_isset(pp->ports_used, port)) {
			__C_DBG("port %d is USED in port pool", port);
			mutex_lock(&pp->free_list_lock);
			unsigned int fport = GPOINTER_TO_UINT(g_queue_pop_head(&pp->free_list));
			if (fport)
				bit_array_clear(pp->free_list_used, fport);
			mutex_unlock(&pp->free_list_lock);
			if (fport) {
				port = fport;
				__C_DBG("Picked port %u from free list", port);
			}
		} else {
			__C_DBG("port %d is NOT USED in port pool", port);
		}
		
		......	

		if (get_port(sk, port++, spec, label))
			goto release_restart;		
			
		......
}

When the program continues to execute for a while, Assume that consecutive_ports in the above function __get_consecutive_ports (port += PORT_RANDOM_MIN + (ssl_random() % (PORT_RANDOM_MAX-PORT_RANDOM_MIN)) results in a port
It is exactly A, and then bit_array_set(pp->ports_used, port) in the subsequent get_port function sets its being used flag. However, port A has not been cleared in free_list_used, and the program continues
Consecutive_ports again in the function __get_consecutive_ports for a period of time, assuming that the port that needs to be applied for is B, but if then if (bit_array_isset(pp->ports_used, port)) is valid, then free_list is called
If an idle port is accessed, and port A is in use, it will fail. The fix code I've added is designed to handle the situation described above.

Thank you (to google Translate)

[rfuchs]

Ok, I think I understand. But I'm not sure this approach is correct.

The purpose of the bit field free_list_used is to prevent duplicate entries in the queue free_list. We track which ports are already in free_list through the bit field, and don't add a new entry into free_list if it's already set as used in free_list_used.

If you clear a bit in free_list_used without actually removing the entry from free_list, then you may end up with a free_list that keeps growing into infinity, as the same ports get added to it over and over again.

So I think a different approach is needed here. Perhaps the entire free_list can be abandoned and replaced by something using clz() or something similar.

[button-chen]

Thank you for your reply, I think your approach is correct, looking forward to optimizing the code

[button-chen]

close

[rfuchs]

Fixed/improved as of c024b54