GNOME Bugzilla – Bug 673083
soup_socket_get_remote_address, get_local_address assume errors can't happen
Last modified: 2012-08-06 14:34:41 UTC
soup_socket_get_remote_address() assumes that priv->gsock can never be NULL, and that g_socket_get_remote_address() can't ever return NULL. However, priv->gsock appears to be NULL before connection or after disconnection. Meanwhile, behind the scenes, g_socket_get_remote_address() calls getpeername() which is documented by POSIX.1-2008 as failing for various reasons, including: - The socket has been shut down - The socket is not connected or otherwise has not had the peer pre-specified - Insufficient resources were available in the system to complete the call soup_socket_get_local_address() has the same problem, with getsockname() instead of getpeername(); so do their corresponding properties. Before porting to GSocketConnection, the corresponding bug already existed (the old code assumed that priv->sockfd could never be -1 and getpeername()/getsockname() could never fail). Unfortunately, soup_socket_get_remote_address() doesn't raise a GError, so it's impossible to pass the error on. The best it can ever do would be to return either NULL or an otherwise invalid SoupAddress (perhaps 0.0.0.0?), either of which could break unsuspecting applications. As far as I can tell, the current failure mode would be to critical a lot and return NULL. Perhaps this method should be deprecated in favour of a version that returns a GSocketAddress or raises a GError?
(In reply to comment #0) > Unfortunately, soup_socket_get_remote_address() doesn't raise a GError, so it's > impossible to pass the error on. The best it can ever do would be to return > either NULL or an otherwise invalid SoupAddress (perhaps 0.0.0.0?), either of > which could break unsuspecting applications. As far as I can tell, the current > failure mode would be to critical a lot and return NULL. Nope, it criticals a lot and then returns a SoupAddress corresponding to stack garbage. Which (modulo the criticals) is what it did before as well. That behavior wasn't intentional, but it's implicit that you're not supposed to call this method on a non-connected socket. Adding g_return_val_if_fail()s to enforce that would be fine. Returning NULL in this case does not seem like a big problem to me, since any app that was mistakenly calling get_remote_address in this case before couldn't possibly have been doing what it's author intended anyway. > Perhaps this method should be deprecated in favour of a version that returns a > GSocketAddress or raises a GError? I don't think that's really needed. The long-term plan is for SoupSocket and SoupAddress to disappear entirely anyway.
Created attachment 210883 [details] [review] SoupSocket: local, remote address are undefined if unconnected Warn and return NULL deterministically, rather than warning and returning uninitialized stack garbage, but document it as "undefined"; these methods were never meant to be valid in this situation, apparently. --- How's this? (Untested, so far.) The g_warning() calls could equally well be g_critical(), or have prefixes that aren't G_STRLOC, or whatever.
Created attachment 210885 [details] [review] SoupSocket: local, remote address are undefined if unconnected Warn and return NULL deterministically, rather than warning and returning uninitialized stack garbage, but document it as "undefined"; these methods were never meant to be valid in this situation, apparently. --- Fixed patch using error->message, not error, as the g_warning argument
Created attachment 210886 [details] [review] Add a simple regression test for unbound sockets
Comment on attachment 210886 [details] [review] Add a simple regression test for unbound sockets ok
pushed. thanks for the patches Attachment 210886 [details] pushed as a0ceee0 - Add a simple regression test for unbound sockets