Re: Errors in bittorent

Some comments ...

Witold Filipczyk <[email protected]> wrote Sat, Aug 30, 2008:
> commit b589f19b73c65621c0a8582199509f58dbcac09f
> Author:     Witold Filipczyk <[email protected]>
> AuthorDate: Sat Aug 30 10:52:00 2008 +0200
> Commit:     Witold Filipczyk <nobody@nowhere>
> CommitDate: Sat Aug 30 10:52:00 2008 +0200
> 
>     An issue with bittorrent.
>     
>     It was possible that the reference to the automatic variable
>     uri of the make_bittorrent_peer_connection trashed the stack.
>     In addition done_uri crashed because uri->string was NULL.
>     
>     Now uri is allocated and unlocked to avoid memleak.
> 
> diff --git a/src/protocol/bittorrent/peerconnect.c b/src/protocol/bittorrent/peerconnect.c
> index aeafbf3..5f65e68 100644
> --- a/src/protocol/bittorrent/peerconnect.c
> +++ b/src/protocol/bittorrent/peerconnect.c
> @@ -20,6 +20,7 @@
>  #include "elinks.h"
>  
>  #include "config/options.h"
> +#include "main/object.h"
>  #include "main/select.h"
>  #include "main/timer.h"
>  #include "network/connection.h"
> @@ -271,9 +272,12 @@ enum bittorrent_state
>  make_bittorrent_peer_connection(struct bittorrent_connection *bittorrent,
>  				struct bittorrent_peer *peer_info)
>  {
> -	struct uri uri;
> +	struct string uri_string;
> +	struct uri *uri;
>  	struct bittorrent_peer_connection *peer;
> -	unsigned char port[5];
> +	unsigned char port[6];
> +	int ip_start, port_start;
> +	int port_length;
>  
>  	peer = init_bittorrent_peer_connection(-1);
>  	if (!peer) return BITTORRENT_STATE_OUT_OF_MEM;
> @@ -296,14 +300,32 @@ make_bittorrent_peer_connection(struct bittorrent_connection *bittorrent,
>  	/* FIXME: Rather change the make_connection() interface. This is an ugly
>  	 * hack. */
>  	/* FIXME: Set the ipv6 flag iff ... */
> -	memset(&uri, 0, sizeof(uri));
> -	uri.protocol = PROTOCOL_BITTORRENT;
> -	uri.host     = peer_info->ip;
> -	uri.hostlen  = strlen(peer_info->ip);
> -	uri.port     = port;
> -	uri.portlen  = snprintf(port, sizeof(port), "%u", peer_info->port);
> -
> -	make_connection(peer->socket, &uri, send_bittorrent_peer_handshake, 1);
> +	if (!init_string(&uri_string)) {
> +		done_bittorrent_peer_connection(peer);
> +		return BITTORRENT_STATE_OUT_OF_MEM;
> +	}
> +	add_to_string(&uri_string, "bittorrent:");
> +	ip_start = uri_string.length;
> +	add_to_string(&uri_string, peer_info->ip);
> +	add_char_to_string(&uri_string, ':');
> +	port_start = uri_string.length;
> +
> +	port_length = snprintf(port, sizeof(port), "%u", peer_info->port);
> +	add_bytes_to_string(&uri_string, port, port_length);
> +	uri = get_uri(uri_string.source, URI_BASE);
> +	done_string(&uri_string);
> +	if (!uri) {
> +		done_bittorrent_peer_connection(peer);
> +		return BITTORRENT_STATE_OUT_OF_MEM;
> +	}
> +
> +	uri->host = uri->string + ip_start;
> +	uri->hostlen = port_start - ip_start - 1;
> +	uri->port = uri->string + port_start;
> +	uri->portlen = port_length;
> +	/* Do not lock it. */
> +	object_unlock(uri);
> +	make_connection(peer->socket, uri, send_bittorrent_peer_handshake, 1);
>  
>  	return BITTORRENT_STATE_OK;
>  }


Instead of fixing this hack, why don't you add a setup_connection() that
make_connection() can use as a wrapper and be done with this. Or if this
is "bittorrent" URI thing actually works, why do you mess with the URI struct
yourself? Didn't parse_uri() do all that for you already? Also don't
unluck the URI, call done_uri(uri) after the call to make_connection().

Else your checking of add_..._string() calls is a bit sloppy.

-- 
Jonas Fonseca