From 08807df47bf5b271dbac663224c35a925554fd66 Mon Sep 17 00:00:00 2001 From: ed Date: Wed, 27 Mar 2019 14:43:47 +0000 Subject: [PATCH] Allow valid socket handles of 0 in StreamingSocket and DatagramSocket, add some unit tests and some minor docs cleanup --- modules/juce_core/network/juce_Socket.cpp | 111 +++++++++++++++++++--- modules/juce_core/network/juce_Socket.h | 69 ++++++++------ 2 files changed, 135 insertions(+), 45 deletions(-) diff --git a/modules/juce_core/network/juce_Socket.cpp b/modules/juce_core/network/juce_Socket.cpp index 8196391e36..b9ab138e98 100644 --- a/modules/juce_core/network/juce_Socket.cpp +++ b/modules/juce_core/network/juce_Socket.cpp @@ -87,7 +87,7 @@ namespace SocketHelpers static bool resetSocketOptions (SocketHandle handle, bool isDatagram, bool allowBroadcast) noexcept { - return handle > 0 + return handle >= 0 && setOption (handle, SO_RCVBUF, (int) 65536) && setOption (handle, SO_SNDBUF, (int) 65536) && (isDatagram ? ((! allowBroadcast) || setOption (handle, SO_BROADCAST, (int) 1)) @@ -103,7 +103,7 @@ namespace SocketHelpers #if JUCE_WINDOWS ignoreUnused (portNumber, isListener, readLock); - if (h != (unsigned) SOCKET_ERROR || connected) + if (h != invalidSocket || connected) closesocket (h); // make sure any read process finishes before we delete the socket @@ -122,7 +122,7 @@ namespace SocketHelpers } } - if (h != -1) + if (h >= 0) { // unblock any pending read requests ::shutdown (h, SHUT_RDWR); @@ -147,7 +147,7 @@ namespace SocketHelpers static bool bindSocket (SocketHandle handle, int port, const String& address) noexcept { - if (handle <= 0 || ! isValidPortNumber (port)) + if (handle < 0 || ! isValidPortNumber (port)) return false; struct sockaddr_in addr; @@ -163,7 +163,7 @@ namespace SocketHelpers static int getBoundPort (SocketHandle handle) noexcept { - if (handle > 0) + if (handle >= 0) { struct sockaddr_in addr; socklen_t len = sizeof (addr); @@ -183,7 +183,7 @@ namespace SocketHelpers if (getpeername (handle, (struct sockaddr*) &addr, &len) >= 0) return inet_ntoa (addr.sin_addr); - return String ("0.0.0.0"); + return "0.0.0.0"; } static int readSocket (SocketHandle handle, @@ -281,11 +281,14 @@ namespace SocketHelpers return -1; #else { - int result; + int result = 0; - while ((result = select (h + 1, prset, pwset, nullptr, timeoutp)) < 0 - && errno == EINTR) + for (;;) { + result = select (h + 1, prset, pwset, nullptr, timeoutp); + + if (result >= 0 || errno != EINTR) + break; } if (result < 0) @@ -501,7 +504,8 @@ bool StreamingSocket::connect (const String& remoteHostName, int remotePortNumbe if (isListener) { - jassertfalse; // a listener socket can't connect to another one! + // a listener socket can't connect to another one! + jassertfalse; return false; } @@ -515,7 +519,10 @@ bool StreamingSocket::connect (const String& remoteHostName, int remotePortNumbe connected = SocketHelpers::connectSocket (handle, readLock, remoteHostName, remotePortNumber, timeOutMillisecs); - if (! (connected && SocketHelpers::resetSocketOptions (handle, false, false))) + if (! connected) + return false; + + if (! SocketHelpers::resetSocketOptions (handle, false, false)) { close(); return false; @@ -526,7 +533,8 @@ bool StreamingSocket::connect (const String& remoteHostName, int remotePortNumbe void StreamingSocket::close() { - SocketHelpers::closeSocket (handle, readLock, isListener, portNumber, connected); + if (handle >= 0) + SocketHelpers::closeSocket (handle, readLock, isListener, portNumber, connected); hostName.clear(); portNumber = 0; @@ -631,8 +639,11 @@ void DatagramSocket::shutdown() std::atomic handleCopy { handle.load() }; handle = -1; + std::atomic connected { false }; SocketHelpers::closeSocket (handleCopy, readLock, false, 0, connected); + + isBound = false; } bool DatagramSocket::bindToPort (int port) @@ -644,6 +655,9 @@ bool DatagramSocket::bindToPort (int port, const String& addr) { jassert (SocketHelpers::isValidPortNumber (port)); + if (handle < 0) + return false; + if (SocketHelpers::bindSocket (handle, port, addr)) { isBound = true; @@ -722,7 +736,7 @@ int DatagramSocket::write (const String& remoteHostname, int remotePortNumber, bool DatagramSocket::joinMulticast (const String& multicastIPAddress) { - if (! isBound || handle < 0) + if (handle < 0 || ! isBound) return false; return SocketHelpers::multicast (handle, multicastIPAddress, lastBindAddress, true); @@ -730,7 +744,7 @@ bool DatagramSocket::joinMulticast (const String& multicastIPAddress) bool DatagramSocket::leaveMulticast (const String& multicastIPAddress) { - if (! isBound || handle < 0) + if (handle < 0 || ! isBound) return false; return SocketHelpers::multicast (handle, multicastIPAddress, lastBindAddress, false); @@ -738,7 +752,7 @@ bool DatagramSocket::leaveMulticast (const String& multicastIPAddress) bool DatagramSocket::setMulticastLoopbackEnabled (bool enable) { - if (! isBound || handle < 0) + if (handle < 0 || ! isBound) return false; return SocketHelpers::setOption (handle, IPPROTO_IP, IP_MULTICAST_LOOP, enable); @@ -766,4 +780,71 @@ bool DatagramSocket::setEnablePortReuse (bool enabled) #pragma warning (pop) #endif +//============================================================================== +#if JUCE_UNIT_TESTS + +struct SocketTests : public UnitTest +{ + SocketTests() + : UnitTest ("Sockets", "Networking") + { + } + + void runTest() override + { + auto localHost = IPAddress::local(); + int portNum = 12345; + + beginTest ("StreamingSocket"); + { + StreamingSocket socketServer; + + expect (socketServer.isConnected() == false); + expect (socketServer.getHostName().isEmpty()); + expect (socketServer.getBoundPort() == -1); + expect (socketServer.getRawSocketHandle() == invalidSocket); + + expect (socketServer.createListener (portNum, localHost.toString())); + + StreamingSocket socket; + + expect (socket.connect (localHost.toString(), portNum)); + + expect (socket.isConnected() == true); + expect (socket.getHostName() == localHost.toString()); + expect (socket.getBoundPort() != -1); + expect (socket.getRawSocketHandle() != invalidSocket); + + socket.close(); + + expect (socket.isConnected() == false); + expect (socket.getHostName().isEmpty()); + expect (socket.getBoundPort() == -1); + expect (socket.getRawSocketHandle() == invalidSocket); + } + + beginTest ("DatagramSocket"); + { + DatagramSocket socket; + + expect (socket.getBoundPort() == -1); + expect (socket.getRawSocketHandle() != invalidSocket); + + expect (socket.bindToPort (portNum, localHost.toString())); + + expect (socket.getBoundPort() == portNum); + expect (socket.getRawSocketHandle() != invalidSocket); + + socket.shutdown(); + + expect (socket.getBoundPort() == -1); + expect (socket.getRawSocketHandle() == invalidSocket); + } + } +}; + +static SocketTests socketTests; + +#endif + } // namespace juce diff --git a/modules/juce_core/network/juce_Socket.h b/modules/juce_core/network/juce_Socket.h index 524554ee55..3e337d77f8 100644 --- a/modules/juce_core/network/juce_Socket.h +++ b/modules/juce_core/network/juce_Socket.h @@ -55,8 +55,8 @@ public: //============================================================================== /** Binds the socket to the specified local port. - @returns true on success; false may indicate that another socket is already bound - on the same port + @returns true on success; false may indicate that another socket is already bound + on the same port */ bool bindToPort (int localPortNumber); @@ -66,8 +66,9 @@ public: as well. This is useful if you would like to bind your socket to a specific network adapter. Note that localAddress must be an IP address assigned to one of your network address otherwise this function will fail. - @returns true on success; false may indicate that another socket is already bound - on the same port + + @returns true on success; false may indicate that another socket is already bound + on the same port @see bindToPort(int localPortNumber), IPAddress::getAllAddresses */ bool bindToPort (int localPortNumber, const String& localAddress); @@ -76,7 +77,9 @@ public: This is useful if you need to know to which port the OS has actually bound your socket when calling the constructor or bindToPort with zero as the - localPortNumber argument. Returns -1 if the function fails. + localPortNumber argument. + + @returns -1 if the function fails */ int getBoundPort() const noexcept; @@ -85,7 +88,7 @@ public: If timeOutMillisecs is 0, then this method will block until the operating system rejects the connection (which could take a long time). - @returns true if it succeeds. + @returns true if it succeeds, false if otherwise @see isConnected */ bool connect (const String& remoteHostname, @@ -119,8 +122,8 @@ public: If the timeout is < 0, it will wait forever, or else will give up after the specified time. - If the socket is ready on return, this returns 1. If it times-out before - the socket becomes ready, it returns 0. If an error occurs, it returns -1. + @returns 1 if the socket is ready on return, 0 if it times-out before + the socket becomes ready, or -1 if an error occurs */ int waitUntilReady (bool readyForReading, int timeoutMsecs); @@ -131,7 +134,7 @@ public: flag is false, the method will return as much data as is currently available without blocking. - @returns the number of bytes read, or -1 if there was an error. + @returns the number of bytes read, or -1 if there was an error @see waitUntilReady */ int read (void* destBuffer, int maxBytesToRead, @@ -142,7 +145,7 @@ public: Note that this method will block unless you have checked the socket is ready for writing before calling it (see the waitUntilReady() method). - @returns the number of bytes written, or -1 if there was an error. + @returns the number of bytes written, or -1 if there was an error */ int write (const void* sourceBuffer, int numBytesToWrite); @@ -156,8 +159,8 @@ public: @param portNumber the port number to listen on @param localHostName the interface address to listen on - pass an empty string to listen on all addresses - @returns true if it manages to open the socket successfully. + @returns true if it manages to open the socket successfully @see waitForNextConnection */ bool createListener (int portNumber, const String& localHostName = String()); @@ -202,8 +205,7 @@ class JUCE_API DatagramSocket final { public: //============================================================================== - /** - Creates a datagram socket. + /** Creates a datagram socket. You first need to bind this socket to a port with bindToPort if you intend to read from this socket. @@ -223,8 +225,8 @@ public: The localPortNumber is the port on which to bind this socket. If this value is 0, the port number is assigned by the operating system. - @returns true on success; false may indicate that another socket is already bound - on the same port + @returns true on success; false may indicate that another socket is already bound + on the same port */ bool bindToPort (int localPortNumber); @@ -234,8 +236,9 @@ public: as well. This is useful if you would like to bind your socket to a specific network adapter. Note that localAddress must be an IP address assigned to one of your network address otherwise this function will fail. - @returns true on success; false may indicate that another socket is already bound - on the same port + + @returns true on success; false may indicate that another socket is already bound + on the same port @see bindToPort(int localPortNumber), IPAddress::getAllAddresses */ bool bindToPort (int localPortNumber, const String& localAddress); @@ -245,7 +248,8 @@ public: This is useful if you need to know to which port the OS has actually bound your socket when bindToPort was called with zero. - Returns -1 if the socket didn't bind to any port yet or an error occurred. */ + @returns -1 if the socket didn't bind to any port yet or an error occurred + */ int getBoundPort() const noexcept; /** Returns the OS's socket handle that's currently open. */ @@ -260,8 +264,8 @@ public: If the timeout is < 0, it will wait forever, or else will give up after the specified time. - If the socket is ready on return, this returns 1. If it times-out before - the socket becomes ready, it returns 0. If an error occurs, it returns -1. + @returns 1 if the socket is ready on return, 0 if it times-out before the + socket becomes ready, or -1 if an error occurs */ int waitUntilReady (bool readyForReading, int timeoutMsecs); @@ -272,7 +276,7 @@ public: flag is false, the method will return as much data as is currently available without blocking. - @returns the number of bytes read, or -1 if there was an error. + @returns the number of bytes read, or -1 if there was an error @see waitUntilReady */ int read (void* destBuffer, int maxBytesToRead, @@ -285,8 +289,8 @@ public: flag is false, the method will return as much data as is currently available without blocking. - @returns the number of bytes read, or -1 if there was an error. On a successful - result, the senderIPAddress value will be set to the IP of the sender. + @returns the number of bytes read, or -1 if there was an error. On a successful + result, the senderIPAddress value will be set to the IP of the sender @see waitUntilReady */ int read (void* destBuffer, int maxBytesToRead, @@ -298,7 +302,7 @@ public: Note that this method will block unless you have checked the socket is ready for writing before calling it (see the waitUntilReady() method). - @returns the number of bytes written, or -1 if there was an error. + @returns the number of bytes written, or -1 if there was an error */ int write (const String& remoteHostname, int remotePortNumber, const void* sourceBuffer, int numBytesToWrite); @@ -306,8 +310,10 @@ public: /** Closes the underlying socket object. Closes the underlying socket object and aborts any read or write operations. - Note that all other methods will return an error after this call. This - method is useful if another thread is blocking in a read/write call and you + Note that all other methods will return an error after this call and the object + cannot be re-used. + + This method is useful if another thread is blocking in a read/write call and you would like to abort the read/write thread. Simply deleting the socket object without calling shutdown may cause a race-condition where the read/write returns just before the socket is deleted and the subsequent read/write would @@ -319,17 +325,20 @@ public: //============================================================================== /** Join a multicast group. - @returns true if it succeeds. + + @returns true if it succeeds */ bool joinMulticast (const String& multicastIPAddress); /** Leave a multicast group. - @returns true if it succeeds. + + @returns true if it succeeds */ bool leaveMulticast (const String& multicastIPAddress); /** Enables or disables multicast loopback. - @returns true if it succeeds. + + @returns true if it succeeds */ bool setMulticastLoopbackEnabled (bool enableLoopback); @@ -340,7 +349,7 @@ public: Do not use this if your socket handles sensitive data as it could be read by any, possibly malicious, third-party apps. - Returns true on success. + @returns true on success */ bool setEnablePortReuse (bool enabled);