Re: [Openbts-discuss] [PATCH 2/2] common: Add mandatory length field to UDP receive calls

[Michael I. <mic...@ra...>]

Hi Tom,

Thanks for the patches. I’ve applied them but OpenBTS now fails to build with SIP2Interface.cpp complaining about the altered UDPSocket::read signature.

Could we not implement this in another way? Most of the altered calls in transceiver pass in sizeof(buf) for length. I suggest we keep the existing signature and default it to calculate a logical length in this way. Then we add the signature you’ve provided to improve the length specification when needed.

Does that make sense? I like the added check but would like existing calls to UDPSocket::read to keep working in SMQueue, SIPAuthServe, etc.


Scoping the transceiver socket to loopback is good as well. I’m not aware of any systems out there with the transceiver and openbts processes running on physically different boxes.

Regards,
-Michael

> On May 3, 2016, at 2:56 AM, Tom Tsou <tom...@et...> wrote:
> 
> Current UDP receive reads up to MAX_UDP_LENGTH bytes into the
> passed in buffer, which may lead to buffer overflow if the
> write buffer is of insufficient size.
> 
> Add mandatory length argument to UDP socket receive calls.
> 
> Reported-by: Simone Margaritelli <si...@zi...>
> Signed-off-by: Tom Tsou <tom...@et...>
> ---
> Sockets.cpp     | 22 +++++++++-------------
> Sockets.h       |  4 ++--
> SocketsTest.cpp |  4 ++--
> 3 files changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/Sockets.cpp b/Sockets.cpp
> index bb00e9f..6880c3e 100644
> --- a/Sockets.cpp
> +++ b/Sockets.cpp
> @@ -199,25 +199,21 @@ int DatagramSocket::send(const struct sockaddr* dest, const char * message)
> 	return send(dest,message,length);
> }
> 
> -
> -
> -
> -
> -int DatagramSocket::read(char* buffer)
> +int DatagramSocket::read(char* buffer, size_t length)
> {
> -	socklen_t temp_len = sizeof(mSource);
> -	int length = recvfrom(mSocketFD, (void*)buffer, MAX_UDP_LENGTH, 0,
> -	    (struct sockaddr*)&mSource,&temp_len);
> -	if ((length==-1) && (errno!=EAGAIN)) {
> +	socklen_t addr_len = sizeof(mSource);
> +	int rd_length = recvfrom(mSocketFD, (void *) buffer, length, 0,
> +		(struct sockaddr*) &mSource, &addr_len);
> +
> +	if ((rd_length==-1) && (errno!=EAGAIN)) {
> 		perror("DatagramSocket::read() failed");
> 		devassert(0);
> 		throw SocketError();
> 	}
> -	return length;
> +	return rd_length;
> }
> 
> -
> -int DatagramSocket::read(char* buffer, unsigned timeout)
> +int DatagramSocket::read(char* buffer, size_t length, unsigned timeout)
> {
> 	fd_set fds;
> 	FD_ZERO(&fds);
> @@ -232,7 +228,7 @@ int DatagramSocket::read(char* buffer, unsigned timeout)
> 		throw SocketError();
> 	}
> 	if (sel==0) return -1;
> -	if (FD_ISSET(mSocketFD,&fds)) return read(buffer);
> +	if (FD_ISSET(mSocketFD,&fds)) return read(buffer, length);
> 	return -1;
> }
> 
> diff --git a/Sockets.h b/Sockets.h
> index 213fcb7..365b745 100644
> --- a/Sockets.h
> +++ b/Sockets.h
> @@ -110,7 +110,7 @@ public:
> 		@param buffer A char[MAX_UDP_LENGTH] procured by the caller.
> 		@return The number of bytes received or -1 on non-blocking pass.
> 	*/
> -	int read(char* buffer);
> +	int read(char* buffer, size_t length);
> 
> 	/**
> 		Receive a packet with a timeout.
> @@ -118,7 +118,7 @@ public:
> 		@param maximum wait time in milliseconds
> 		@return The number of bytes received or -1 on timeout.
> 	*/
> -	int read(char* buffer, unsigned timeout);
> +	int read(char* buffer, size_t length, unsigned timeout);
> 
> 
> 	/** Send a packet to a given destination, other than the default. */
> diff --git a/SocketsTest.cpp b/SocketsTest.cpp
> index ef2fddf..7ed2744 100644
> --- a/SocketsTest.cpp
> +++ b/SocketsTest.cpp
> @@ -45,7 +45,7 @@ void *testReaderIP(void *)
> 	int rc = 0;
> 	while (rc<gNumToSend) {
> 		char buf[MAX_UDP_LENGTH];
> -		int count = readSocket.read(buf);
> +		int count = readSocket.read(buf, MAX_UDP_LENGTH);
> 		if (count>0) {
> 			COUT("read: " << buf);
> 			rc++;
> @@ -65,7 +65,7 @@ void *testReaderUnix(void *)
> 	int rc = 0;
> 	while (rc<gNumToSend) {
> 		char buf[MAX_UDP_LENGTH];
> -		int count = readSocket.read(buf);
> +		int count = readSocket.read(buf, MAX_UDP_LENGTH);
> 		if (count>0) {
> 			COUT("read: " << buf);
> 			rc++;
> -- 
> 2.4.11
>