Call WSAGetLastError immediately after the potentially failing call.

net/socket/tcp_socket_win.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/socket/tcp_socket.h"
 #include "net/socket/tcp_socket_win.h"
 
 #include <errno.h>
 #include <mstcpip.h>
 
@@ skipping 22 matching lines @@
 
 namespace net {
 
 namespace {
 
 const int kTCPKeepAliveSeconds = 45;
 
 int SetSocketReceiveBufferSize(SOCKET socket, int32_t size) {
   int rv = setsockopt(socket, SOL_SOCKET, SO_RCVBUF,
                       reinterpret_cast<const char*>(&size), sizeof(size));
-  int net_error = (rv == 0) ? OK : MapSystemError(WSAGetLastError());
+  int os_error = WSAGetLastError();
+  int net_error = (rv == 0) ? OK : MapSystemError(os_error);
   DCHECK(!rv) << "Could not set socket receive buffer size: " << net_error;
   return net_error;
 }
 
 int SetSocketSendBufferSize(SOCKET socket, int32_t size) {
   int rv = setsockopt(socket, SOL_SOCKET, SO_SNDBUF,
                       reinterpret_cast<const char*>(&size), sizeof(size));
-  int net_error = (rv == 0) ? OK : MapSystemError(WSAGetLastError());
+  int os_error = WSAGetLastError();

[eroman, 2016/11/29 19:51:00]

For these two cases, does the re-ordering make a difference, or is it purely for
consistency? I am OK with this either way

[Sigurður Ásgeirsson, 2016/11/29 20:27:55]

This is purely for consistency. Note that C++ can be a trifling implicit, so in
the unlikely case if e.g. MapSystemError in the future starts constructing
temporaries around the argument, that temporary could smite your last error
value.

+  int net_error = (rv == 0) ? OK : MapSystemError(os_error);
   DCHECK(!rv) << "Could not set socket send buffer size: " << net_error;
   return net_error;
 }
 
 // Disable Nagle.
 // Enable TCP Keep-Alive to prevent NAT routers from timing out TCP
 // connections. See http://crbug.com/27400 for details.
 bool SetTCPKeepAlive(SOCKET socket, BOOL enable, int delay_secs) {
   unsigned delay = delay_secs * 1000;
   struct tcp_keepalive keepalive_vals = {
       enable ? 1u : 0u,  // TCP keep-alive on.
       delay,  // Delay seconds before sending first TCP keep-alive packet.
       delay,  // Delay seconds between sending TCP keep-alive packets.
   };
   DWORD bytes_returned = 0xABAB;
   int rv = WSAIoctl(socket, SIO_KEEPALIVE_VALS, &keepalive_vals,
                     sizeof(keepalive_vals), NULL, 0,
                     &bytes_returned, NULL, NULL);
+  int os_error = WSAGetLastError();
   DCHECK(!rv) << "Could not enable TCP Keep-Alive for socket: " << socket
-              << " [error: " << WSAGetLastError() << "].";
+              << " [error: " << os_error << "].";
 
   // Disregard any failure in disabling nagle or enabling TCP Keep-Alive.
   return rv == 0;
 }
 
 int MapConnectError(int os_error) {
   switch (os_error) {
     // connect fails with WSAEACCES when Windows Firewall blocks the
     // connection.
     case WSAEACCES:
@@ skipping 187 matching lines @@
   Close();
   net_log_.EndEvent(NetLogEventType::SOCKET_ALIVE);
 }
 
 int TCPSocketWin::Open(AddressFamily family) {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(socket_, INVALID_SOCKET);
 
   socket_ = CreatePlatformSocket(ConvertAddressFamily(family), SOCK_STREAM,
                                  IPPROTO_TCP);
+  int os_error = WSAGetLastError();

[eroman, 2016/11/29 19:51:00]

Same issue as for base::SetNonBlocking() -- relies on CreatePlatformSocket() not
clobbering the last error.

A reasonable follow-up could be to add an "int* os_error" out-parameter to
CreatePlatformSocket().

[Sigurður Ásgeirsson, 2016/11/29 20:27:55]

I don't know whether this works the same on other platforms, and in any case I'm
mostly concerned with fixing the one real problem(TM) here to get my CL landed.

   if (socket_ == INVALID_SOCKET) {
     PLOG(ERROR) << "CreatePlatformSocket() returned an error";
-    return MapSystemError(WSAGetLastError());
+    return MapSystemError(os_error);
   }
 
   if (!base::SetNonBlocking(socket_)) {
-    int result = MapSystemError(WSAGetLastError());
+    os_error = WSAGetLastError();
+    int result = MapSystemError(os_error);
     Close();
     return result;
   }
 
   return OK;
 }
 
 int TCPSocketWin::AdoptConnectedSocket(SOCKET socket,
                                        const IPEndPoint& peer_address) {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(socket_, INVALID_SOCKET);
   DCHECK(!core_.get());
 
   socket_ = socket;
 
   if (!base::SetNonBlocking(socket_)) {
-    int result = MapSystemError(WSAGetLastError());
+    int os_error = WSAGetLastError();

[eroman, 2016/11/29 19:51:00]

Given this is called 3 times, I would suggest adding a file-local wrapper
function, say
    bool SetNonBlocking(..., int* os_error);

Similar to CreatePlatformSocket(), could push this interface to the base
implementation too.

[Sigurður Ásgeirsson, 2016/11/29 20:27:55]

Done.

+    int result = MapSystemError(os_error);
     Close();
     return result;
   }
 
   core_ = new Core(this);
   peer_address_.reset(new IPEndPoint(peer_address));
 
   return OK;
 }
 
 int TCPSocketWin::AdoptListenSocket(SOCKET socket) {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(socket_, INVALID_SOCKET);
 
   socket_ = socket;
 
   if (!base::SetNonBlocking(socket_)) {
-    int result = MapSystemError(WSAGetLastError());
+    int os_error = WSAGetLastError();
+    int result = MapSystemError(os_error);
     Close();
     return result;
   }
 
   // |core_| is not needed for sockets that are used to accept connections.
   // The operation here is more like Open but with an existing socket.
 
   return OK;
 }
 
 int TCPSocketWin::Bind(const IPEndPoint& address) {
   DCHECK(CalledOnValidThread());
   DCHECK_NE(socket_, INVALID_SOCKET);
 
   SockaddrStorage storage;
   if (!address.ToSockAddr(storage.addr, &storage.addr_len))
     return ERR_ADDRESS_INVALID;
 
   int result = bind(socket_, storage.addr, storage.addr_len);
+  int os_error = WSAGetLastError();
   if (result < 0) {
     PLOG(ERROR) << "bind() returned an error";
-    return MapSystemError(WSAGetLastError());
+    return MapSystemError(os_error);
   }
 
   return OK;
 }
 
 int TCPSocketWin::Listen(int backlog) {
   DCHECK(CalledOnValidThread());
   DCHECK_GT(backlog, 0);
   DCHECK_NE(socket_, INVALID_SOCKET);
   DCHECK_EQ(accept_event_, WSA_INVALID_EVENT);
 
   accept_event_ = WSACreateEvent();
+  int os_error = WSAGetLastError();
   if (accept_event_ == WSA_INVALID_EVENT) {
     PLOG(ERROR) << "WSACreateEvent()";
-    return MapSystemError(WSAGetLastError());
+    return MapSystemError(os_error);
   }
 
   int result = listen(socket_, backlog);
+  os_error = WSAGetLastError();
   if (result < 0) {
     PLOG(ERROR) << "listen() returned an error";
-    return MapSystemError(WSAGetLastError());
+    return MapSystemError(os_error);
   }
 
   return OK;
 }
 
 int TCPSocketWin::Accept(std::unique_ptr<TCPSocketWin>* socket,
                          IPEndPoint* address,
                          const CompletionCallback& callback) {
   DCHECK(CalledOnValidThread());
   DCHECK(socket);
@@ skipping 57 matching lines @@
 
   if (socket_ == INVALID_SOCKET || waiting_connect_)
     return false;
 
   if (waiting_read_)
     return true;
 
   // Check if connection is alive.
   char c;
   int rv = recv(socket_, &c, 1, MSG_PEEK);
+  int os_error = WSAGetLastError();
   if (rv == 0)
     return false;
-  if (rv == SOCKET_ERROR && WSAGetLastError() != WSAEWOULDBLOCK)
+  if (rv == SOCKET_ERROR && os_error != WSAEWOULDBLOCK)
     return false;
 
   return true;
 }
 
 bool TCPSocketWin::IsConnectedAndIdle() const {
   DCHECK(CalledOnValidThread());
 
   if (socket_ == INVALID_SOCKET || waiting_connect_)
     return false;
 
   if (waiting_read_)
     return true;
 
   // Check if connection is alive and we haven't received any data
   // unexpectedly.
   char c;
   int rv = recv(socket_, &c, 1, MSG_PEEK);
+  int os_error = WSAGetLastError();
   if (rv >= 0)
     return false;
-  if (WSAGetLastError() != WSAEWOULDBLOCK)
+  if (os_error != WSAEWOULDBLOCK)
     return false;
 
   return true;
 }
 
 int TCPSocketWin::Read(IOBuffer* buf,
                        int buf_len,
                        const CompletionCallback& callback) {
   DCHECK(CalledOnValidThread());
   DCHECK_NE(socket_, INVALID_SOCKET);
@@ skipping 16 matching lines @@
 
   WSABUF write_buffer;
   write_buffer.len = buf_len;
   write_buffer.buf = buf->data();
 
   // TODO(wtc): Remove the assertion after enough testing.
   AssertEventNotSignaled(core_->write_overlapped_.hEvent);
   DWORD num;
   int rv = WSASend(socket_, &write_buffer, 1, &num, 0,
                    &core_->write_overlapped_, NULL);
+  int os_error = WSAGetLastError();
   if (rv == 0) {
     if (ResetEventIfSignaled(core_->write_overlapped_.hEvent)) {
       rv = static_cast<int>(num);
       if (rv > buf_len || rv < 0) {
         // It seems that some winsock interceptors report that more was written
         // than was available. Treat this as an error.  http://crbug.com/27870
         LOG(ERROR) << "Detected broken LSP: Asked to write " << buf_len
                    << " bytes, but " << rv << " bytes reported.";
         return ERR_WINSOCK_UNEXPECTED_WRITTEN_BYTES;
       }
       net_log_.AddByteTransferEvent(NetLogEventType::SOCKET_BYTES_SENT, rv,
                                     buf->data());
       NetworkActivityMonitor::GetInstance()->IncrementBytesSent(rv);
       return rv;
     }
   } else {
-    int os_error = WSAGetLastError();
     if (os_error != WSA_IO_PENDING) {
       int net_error = MapSystemError(os_error);
       net_log_.AddEvent(NetLogEventType::SOCKET_WRITE_ERROR,
                         CreateNetLogSocketErrorCallback(net_error, os_error));
       return net_error;
     }
   }
   waiting_write_ = true;
   write_callback_ = callback;
   core_->write_iobuffer_ = buf;
   core_->write_buffer_length_ = buf_len;
   core_->WatchForWrite();
   return ERR_IO_PENDING;
 }
 
 int TCPSocketWin::GetLocalAddress(IPEndPoint* address) const {
   DCHECK(CalledOnValidThread());
   DCHECK(address);
 
   SockaddrStorage storage;
-  if (getsockname(socket_, storage.addr, &storage.addr_len))
-    return MapSystemError(WSAGetLastError());
+  if (getsockname(socket_, storage.addr, &storage.addr_len)) {
+    int os_error = WSAGetLastError();
+    return MapSystemError(os_error);
+  }
   if (!address->FromSockAddr(storage.addr, storage.addr_len))
     return ERR_ADDRESS_INVALID;
 
   return OK;
 }
 
 int TCPSocketWin::GetPeerAddress(IPEndPoint* address) const {
   DCHECK(CalledOnValidThread());
   DCHECK(address);
   if (!IsConnected())
@@ skipping 132 matching lines @@
     logging_multiple_connect_attempts_ = false;
   } else {
     NOTREACHED();
   }
 }
 
 int TCPSocketWin::AcceptInternal(std::unique_ptr<TCPSocketWin>* socket,
                                  IPEndPoint* address) {
   SockaddrStorage storage;
   int new_socket = accept(socket_, storage.addr, &storage.addr_len);
+  int os_error = WSAGetLastError();
   if (new_socket < 0) {
-    int net_error = MapSystemError(WSAGetLastError());
+    int net_error = MapSystemError(os_error);
     if (net_error != ERR_IO_PENDING)
       net_log_.EndEventWithNetErrorCode(NetLogEventType::TCP_ACCEPT, net_error);
     return net_error;
   }
 
   IPEndPoint ip_end_point;
   if (!ip_end_point.FromSockAddr(storage.addr, storage.addr_len)) {
     NOTREACHED();
     if (closesocket(new_socket) < 0)
       PLOG(ERROR) << "closesocket";
@@ skipping 52 matching lines @@
 
   // WSAEventSelect sets the socket to non-blocking mode as a side effect.
   // Our connect() and recv() calls require that the socket be non-blocking.
   WSAEventSelect(socket_, core_->read_overlapped_.hEvent, FD_CONNECT);
 
   SockaddrStorage storage;
   if (!peer_address_->ToSockAddr(storage.addr, &storage.addr_len))
     return ERR_ADDRESS_INVALID;
 
   int result;
+  int os_error;
   {
     // TODO(ricea): Remove ScopedTracker below once crbug.com/436634 is fixed.
     tracked_objects::ScopedTracker tracking_profile(
         FROM_HERE_WITH_EXPLICIT_FUNCTION("436634 connect()"));
     result = connect(socket_, storage.addr, storage.addr_len);
+    os_error = WSAGetLastError();
   }
 
   if (!result) {
     // Connected without waiting!
     //
     // The MSDN page for connect says:
     //   With a nonblocking socket, the connection attempt cannot be completed
     //   immediately. In this case, connect will return SOCKET_ERROR, and
     //   WSAGetLastError will return WSAEWOULDBLOCK.
     // which implies that for a nonblocking socket, connect never returns 0.
     // It's not documented whether the event object will be signaled or not
     // if connect does return 0.  So the code below is essentially dead code
     // and we don't know if it's correct.
     NOTREACHED();
 
     if (ResetEventIfSignaled(core_->read_overlapped_.hEvent))
       return OK;
   } else {
-    int os_error = WSAGetLastError();
     if (os_error != WSAEWOULDBLOCK) {
       LOG(ERROR) << "connect failed: " << os_error;
       connect_os_error_ = os_error;
       int rv = MapConnectError(os_error);
       CHECK_NE(ERR_IO_PENDING, rv);
       return rv;
     }
   }
 
   // TODO(ricea): Remove ScopedTracker below once crbug.com/436634 is fixed.
@@ skipping 27 matching lines @@
 void TCPSocketWin::LogConnectEnd(int net_error) {
   if (net_error != OK) {
     net_log_.EndEventWithNetErrorCode(NetLogEventType::TCP_CONNECT, net_error);
     return;
   }
 
   struct sockaddr_storage source_address;
   socklen_t addrlen = sizeof(source_address);
   int rv = getsockname(
       socket_, reinterpret_cast<struct sockaddr*>(&source_address), &addrlen);
+  int os_error = WSAGetLastError();
   if (rv != 0) {
-    LOG(ERROR) << "getsockname() [rv: " << rv
-               << "] error: " << WSAGetLastError();
+    LOG(ERROR) << "getsockname() [rv: " << rv << "] error: " << os_error;
     NOTREACHED();
     net_log_.EndEventWithNetErrorCode(NetLogEventType::TCP_CONNECT, rv);
     return;
   }
 
   net_log_.EndEvent(
       NetLogEventType::TCP_CONNECT,
       CreateNetLogSourceAddressCallback(
           reinterpret_cast<const struct sockaddr*>(&source_address),
           sizeof(source_address)));
 }
 
 int TCPSocketWin::DoRead(IOBuffer* buf, int buf_len,
                          const CompletionCallback& callback) {
   if (!core_->non_blocking_reads_initialized_) {
     WSAEventSelect(socket_, core_->read_overlapped_.hEvent,
                    FD_READ | FD_CLOSE);
     core_->non_blocking_reads_initialized_ = true;
   }
   int rv = recv(socket_, buf->data(), buf_len, 0);
+  int os_error = WSAGetLastError();
   if (rv == SOCKET_ERROR) {
-    int os_error = WSAGetLastError();
     if (os_error != WSAEWOULDBLOCK) {
       int net_error = MapSystemError(os_error);
       net_log_.AddEvent(NetLogEventType::SOCKET_READ_ERROR,
                         CreateNetLogSocketErrorCallback(net_error, os_error));
       return net_error;
     }
   } else {
     net_log_.AddByteTransferEvent(NetLogEventType::SOCKET_BYTES_RECEIVED, rv,
                                   buf->data());
     NetworkActivityMonitor::GetInstance()->IncrementBytesReceived(rv);
     return rv;
   }
 
   waiting_read_ = true;
   read_callback_ = callback;
   core_->read_iobuffer_ = buf;
   core_->read_buffer_length_ = buf_len;
   core_->WatchForRead();
   return ERR_IO_PENDING;
 }
 
 void TCPSocketWin::DidCompleteConnect() {
   DCHECK(waiting_connect_);
   DCHECK(!read_callback_.is_null());
   int result;
 
   WSANETWORKEVENTS events;
   int rv;
+  int os_error = 0;
   {
     // TODO(pkasting): Remove ScopedTracker below once crbug.com/462784 is
     // fixed.
     tracked_objects::ScopedTracker tracking_profile1(
         FROM_HERE_WITH_EXPLICIT_FUNCTION(
             "462784 TCPSocketWin::DidCompleteConnect -> WSAEnumNetworkEvents"));
     rv = WSAEnumNetworkEvents(socket_, core_->read_overlapped_.hEvent, &events);
+    os_error = WSAGetLastError();
   }
-  int os_error = 0;
   if (rv == SOCKET_ERROR) {
     NOTREACHED();
-    os_error = WSAGetLastError();
     result = MapSystemError(os_error);
   } else if (events.lNetworkEvents & FD_CONNECT) {
     os_error = events.iErrorCode[FD_CONNECT_BIT];
     result = MapConnectError(os_error);
   } else {
     NOTREACHED();
     result = ERR_UNEXPECTED;
   }
 
   connect_os_error_ = os_error;
   DoConnectComplete(result);
   waiting_connect_ = false;
 
   // TODO(pkasting): Remove ScopedTracker below once crbug.com/462784 is fixed.
   tracked_objects::ScopedTracker tracking_profile4(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "462784 TCPSocketWin::DidCompleteConnect -> read_callback_"));
   DCHECK_NE(result, ERR_IO_PENDING);
   base::ResetAndReturn(&read_callback_).Run(result);
 }
 
 void TCPSocketWin::DidCompleteWrite() {
   DCHECK(waiting_write_);
   DCHECK(!write_callback_.is_null());
 
   DWORD num_bytes, flags;
   BOOL ok = WSAGetOverlappedResult(socket_, &core_->write_overlapped_,
                                    &num_bytes, FALSE, &flags);
   WSAResetEvent(core_->write_overlapped_.hEvent);
+  int os_error = WSAGetLastError();

[eroman, 2016/11/29 19:51:00]

Based on the conditional where this is used, I think it should be called
immediately after WSAGetOverlappedResult().

[Sigurður Ásgeirsson, 2016/11/29 20:27:55]

Done, though this probably does change semantics in the unlikely case where
WSAResetEvent fails. I don't know whether that's better or worse, as
WSAResetEvent is unlikely to fail except on handle abuse or programming error...

   waiting_write_ = false;
   int rv;
   if (!ok) {
-    int os_error = WSAGetLastError();
     rv = MapSystemError(os_error);
     net_log_.AddEvent(NetLogEventType::SOCKET_WRITE_ERROR,
                       CreateNetLogSocketErrorCallback(rv, os_error));
   } else {
     rv = static_cast<int>(num_bytes);
     if (rv > core_->write_buffer_length_ || rv < 0) {
       // It seems that some winsock interceptors report that more was written
       // than was available. Treat this as an error.  http://crbug.com/27870
       LOG(ERROR) << "Detected broken LSP: Asked to write "
                  << core_->write_buffer_length_ << " bytes, but " << rv
@@ skipping 13 matching lines @@
 }
 
 void TCPSocketWin::DidSignalRead() {
   DCHECK(waiting_read_);
   DCHECK(!read_callback_.is_null());
 
   int os_error = 0;
   WSANETWORKEVENTS network_events;
   int rv = WSAEnumNetworkEvents(socket_, core_->read_overlapped_.hEvent,
                                 &network_events);
+  os_error = WSAGetLastError();
+
   if (rv == SOCKET_ERROR) {
-    os_error = WSAGetLastError();
     rv = MapSystemError(os_error);
   } else if (network_events.lNetworkEvents) {
     // TODO(pkasting): Remove ScopedTracker below once crbug.com/462778 is
     // fixed.
     tracked_objects::ScopedTracker tracking_profile2(
         FROM_HERE_WITH_EXPLICIT_FUNCTION(
             "462778 TCPSocketWin::DidSignalRead -> DoRead"));
     DCHECK_EQ(network_events.lNetworkEvents & ~(FD_READ | FD_CLOSE), 0);
     // If network_events.lNetworkEvents is FD_CLOSE and
     // network_events.iErrorCode[FD_CLOSE_BIT] is 0, it is a graceful
@@ skipping 29 matching lines @@
 }
 
 bool TCPSocketWin::GetEstimatedRoundTripTime(base::TimeDelta* out_rtt) const {
   DCHECK(out_rtt);
   // TODO(bmcquade): Consider implementing using
   // GetPerTcpConnectionEStats/GetPerTcp6ConnectionEStats.
   return false;
 }
 
 }  // namespace net