Menu
▾
▴
Re: [Openbts-discuss] [PATCH 2/2] common: Add mandatory length field to UDP receive calls
|
From: Michael I. <mic...@ra...> - 2016-05-05 10:14:52
|
For anyone else wanting to try these out, this diff will resolve usage of the updated interface.
diff --git a/Peering/Peering.cpp b/Peering/Peering.cpp
index ccf7209..86c980d 100644
--- a/Peering/Peering.cpp
+++ b/Peering/Peering.cpp
@@ -218,7 +218,7 @@ void* PeerInterface::serviceLoop2(void*)
void PeerInterface::drive()
{
- int numRead = mSocket.read(mReadBuffer);
+ int numRead = mSocket.read(mReadBuffer, sizeof(mReadBuffer));
if (numRead<0) {
return;
}
diff --git a/SIP/SIP2Interface.cpp b/SIP/SIP2Interface.cpp
index bf4e9f8..c584d98 100644
--- a/SIP/SIP2Interface.cpp
+++ b/SIP/SIP2Interface.cpp
@@ -669,7 +669,7 @@ void SipInterface::siDrive2()
// All inbound SIP messages go here for processing.
LOG(DEBUG) << "blocking on socket";
- int numRead = mSIPSocket->read(mReadBuffer);
+ int numRead = mSIPSocket->read(mReadBuffer, sizeof(mReadBuffer));
if (numRead<0) {
LOG(ALERT) << "cannot read SIP socket.";
return;
diff --git a/TRXManager/TRXManager.cpp b/TRXManager/TRXManager.cpp
index f99a4e7..c4a8cfd 100644
--- a/TRXManager/TRXManager.cpp
+++ b/TRXManager/TRXManager.cpp
@@ -234,7 +234,7 @@ void ::ARFCNManager::driveRx()
{
// read the message
char buffer[MAX_UDP_LENGTH];
- int msgLen = mDataSocket.read(buffer);
+ int msgLen = mDataSocket.read(buffer, sizeof(buffer));
if (msgLen<=0) SOCKET_ERROR;
// decode
unsigned char *rp = (unsigned char*)buffer;
> On May 5, 2016, at 11:49 AM, Michael Iedema <mic...@ra...> wrote:
>
> 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
>>
>
> ------------------------------------------------------------------------------
> Find and fix application performance issues faster with Applications Manager
> Applications Manager provides deep performance insights into multiple tiers of
> your business applications. It resolves application problems quickly and
> reduces your MTTR. Get your free trial!
> https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
> _______________________________________________
> Openbts-discuss mailing list
> Ope...@li...
> https://lists.sourceforge.net/lists/listinfo/openbts-discuss
|
