why does recv() function always return SOCKET_ERROR in Wine?

[iSuneast]

Hi everyone, I am a newbie here.

I try to transform a program from windows to ubuntu using Wine.

The program is quite simple, just accept a new connection and
at the same time close the previous one.

I find that the program works well in Windows but not in Ubuntu.

The program work well when communicate with the first connection.
But if there is a new connection, I will close the first connection and then
receive "SOCKET_ERROR" from the recv() function.

Code: Select all

waiting for connection
accepted '127.0.0.1'
remote computer is closed
accepted '127.0.0.1'
recv failed: FormatMessage() unable to fetch error text: 317
accepted '127.0.0.1'
recv failed: FormatMessage() unable to fetch error text: 317
....

Here's my code for more details if anyone concern.

Code: Select all

DWORD WINAPI Interactive(LPVOID param) {
	SOCKET remote_socket = *((SOCKET*)param);
	char buf[PKG_SIZE];
	char net_buffer[PKG_SIZE<<1];
	int net_buffer_size = 0;
	while(true) {
		int bytes = recv(remote_socket, buf, PKG_SIZE, 0);
		if(bytes == SOCKET_ERROR) {    // !!!!! always SOCKET_ERROR except the first connection
			ColorWrite(COLOR_ERROR, "recv failed: %s\n", GetLastErrorDetails());
			closesocket(remote_socket);
			return 0;
		} else if(bytes == 0) {
			ColorWrite(COLOR_WARNING, "remote computer is closed\n");
			closesocket(remote_socket);
			return 0;
		} else {
			ParsePackage(buf, bytes, net_buffer, net_buffer_size);
		}
	}

	closesocket(remote_socket);
	return 0;
}

int main() {
	SOCKET socket_listen;
	SOCKADDR_IN sock_addr;
	WinSockManager::GetInst()->NewTcpServer(&socket_listen, &sock_addr, REMOTE_PORT, BACKLOG, true);
	ColorWrite(COLOR_STATUS, "waiting for connection\n");
	SOCKET remote_socket = accept(socket_listen, (SOCKADDR*)&sock_addr, &SOCKADDR_LEN);
	if(remote_socket == INVALID_SOCKET) {
		FatalMessageBox("accept failed. %s\n", GetLastErrorDetails());
	} else {
		ColorWrite(COLOR_OK, "accepted '%s'\n", inet_ntoa(sock_addr.sin_addr));
	}

	while(true) {
		SOCKET tmp = remote_socket;
		HANDLE h_thread = CreateThread(NULL, 0, Interactive, (LPVOID)&tmp, 0, NULL);
		if(h_thread == NULL) {
			ColorWrite(COLOR_ERROR, "CreateThread failed: %s\n", GetLastErrorDetails());
			break;
		}
		
		remote_socket = accept(socket_listen, (SOCKADDR*)&sock_addr, &SOCKADDR_LEN);
		closesocket(tmp);

		if(remote_socket == INVALID_SOCKET) {
			closesocket(socket_listen);
			FatalMessageBox("accept failed. %s\n", GetLastErrorDetails());
		} else {
			ColorWrite(COLOR_OK, "accepted '%s'\n", inet_ntoa(sock_addr.sin_addr));
		}
	}
		
	return 0;
}

Thank you very much in advance for your help.

[iSuneast]

I don't know if it's my program's problem or a bug of wine...

can anyone help me out?

Thank you again.

[DanKegel]

It's a plain old programming error.
closesocket(tmp) is closing the socket before you read from it.

[iSuneast]

It's a plain old programming error.
closesocket(tmp) is closing the socket before you read from it.

Thank you for your help in advance.

Code: Select all

while(true) {
    SOCKET tmp = remote_socket; 
    HANDLE h_thread = CreateThread(NULL, 0, Interactive, (LPVOID)&tmp, 0, NULL); 
    remote_socket = accept(socket_listen, (SOCKADDR*)&sock_addr, &SOCKADDR_LEN); 
    closesocket(tmp); 
}

According to the code above, I think the program just trying to
close the previous socket (tmp) not the new one (remote_socket).

ps. In fact, this program works well in Windows for about half a
year. Recently, the manager wants me to transplant the program to
Linux. I think this is just a piece of cake for wine...

[DanKegel]

You think you're closing the old socket, but what if the old socket
and the new one have the same handle?
Your code breaks on Linux (or any Unix) even if run natively,
without wine.
You're just getting lucky on Windows.

Isn't that closesocket() redundant, anyway? Interactive is already
closing the socket when it's done. Or did you want to interrupt the
Interactive thread? If so, that's not the way to do it on Unix,
closing a socket doesn't wake up someone sleeping on it iirc in linux.

While you're at it, you might consider restructuring that loop
to avoid the duplicated code, e.g. remove the accept above the loop,
and make the loop look like:

Code: Select all

    while (true) {
        sock_addr_len = sizeof(sock_addr);
        remote_socket = accept(socket_listen, (SOCKADDR *) & sock_addr, &sock_addr_len);

        if (remote_socket == INVALID_SOCKET) {
            closesocket(socket_listen);
            FatalMessageBox("accept failed. %s\n", GetLastErrorDetails()); 
       } else { 
            ColorWrite(COLOR_OK, "accepted '%s'\n", inet_ntoa(sock_addr.sin_addr)); 
       } 

        HANDLE h_thread = CreateThread(NULL, 0, Interactive, (LPVOID)&remote_socket, 0, NULL);
}

[iSuneast]

You think you're closing the old socket, but what if the old socket
and the new one have the same handle?
Your code breaks on Linux (or any Unix) even if run natively,
without wine.
You're just getting lucky on Windows.

Amazing! I have never thought about it.
In fact, I used to believe that the accept() function will always return a new handle.
Yeah, you are right, I have to validate the handle before I close it. : )

Isn't that closesocket() redundant, anyway? Interactive is already
closing the socket when it's done. Or did you want to interrupt the
Interactive thread? If so, that's not the way to do it on Unix,
closing a socket doesn't wake up someone sleeping on it iirc in linux.

Indeed, my option is if I close the old socket the old thread will exit
due to the fact that recv() function will return 0, which means that
"remote computer is closed".
I don't mean to interactive the thread. As you know it, the program has to
close the previous connection if a new one is set up.

While you're at it, you might consider restructuring that loop
to avoid the duplicated code, e.g. remove the accept above the loop,
and make the loop look like:

You are right. My code is rather confusing. I will modify it right now.

Thank you DanKegel. : )

[iSuneast]

Yeah~ Thank you for DanKegel's help. I finally find out the bugs.

Obviously, it's quite a silly bug, I don't know why the porgram is able to run in Windows
for about half a year without any problems. Maybe I am a lucky guy?

I restructuring the code like below to resolve the problem:

Code: Select all

	SOCKET *p_previous_socket = NULL;
	while(true) {
		SOCKET *remote_socket = new SOCKET;
		*remote_socket = accept(socket_listen, (SOCKADDR*)&sock_addr, &SOCKADDR_LEN);
		if(*remote_socket == INVALID_SOCKET) {
			FatalMessageBox("accept failed. %s\n", GetLastErrorDetails());
		} else {
			ColorWrite(COLOR_OK, "accepted '%s'\n", inet_ntoa(sock_addr.sin_addr));
		}

		if(p_previous_socket == NULL) {
			p_previous_socket = remote_socket;
		} else {
			if(*p_previous_socket != *remote_socket) {
				closesocket(*p_previous_socket);
				delete p_previous_socket;
				p_previous_socket = remote_socket;
			} else {
				delete p_previous_socket;
				p_previous_socket = NULL;
			}
		}

		HANDLE h_thread = CreateThread(NULL, 0, Interactive, (LPVOID)remote_socket, 0, NULL);
		if(h_thread == NULL) {
			ColorWrite(COLOR_ERROR, "CreateThread failed: %s\n", GetLastErrorDetails());
			break;
		}
	}

Thank DanKegel again for your help.

[DanKegel]

That code looks better, but in general, you should let the thread servicing
a client be the one to close that client's socket. You may need to tell that thread to close the socket and exit using some other mechanism.

Have you checked to see if those old threads still hang around on wine
after you close their sockets? On Windows, closing a socket will wake
up a thread waiting on it, but I'm not sure that happens on Wine.
(IIRC linux doesn't behave that way, and wine uses Linux sockets.)

Also, in general, looking at the value of the old handle is not sufficient;
if it's been closed and some other socket created somewhere else
in the app with the same value, you'll be closing the wrong socket.
(It might work for your app now, and then break a year or two later
when you add more sockets in other threads.)

[iSuneast]

That code looks better, but in general, you should let the thread servicing
a client be the one to close that client's socket. You may need to tell that thread to
close the socket and exit using some other mechanism.

In terms of the recv() function will block the thread, I don’t know
how to cancel the operation. I just wanted to keep it simple and stupid.

Quite honestly, I admit that is not a well design mechanism.
I will try to find a way out and fix it latter. : )

Have you checked to see if those old threads still hang around on wine
after you close their sockets? On Windows, closing a socket will wake
up a thread waiting on it, but I'm not sure that happens on Wine.
(IIRC linux doesn't behave that way, and wine uses Linux sockets.)

I think the wine works well. The old thread will exit with message
"remote computer is closed" as we expected.

I don’t care much about whether the old thread has already existed or not
cause it is relatively not that kind of importance despite of the
influence performance. : (

Additionally, the main feature of this program is make sure that,
there is always at most one valid connection being enabled.

Also, in general, looking at the value of the old handle is not sufficient;
if it's been closed and some other socket created somewhere else
in the app with the same value, you'll be closing the wrong socket.
(It might work for your app now, and then break a year or two later
when you add more sockets in other threads.)

Oh, my god... You remind me that I forget to set the “p_previous_socket”
to “NULL” after I delete it. Now, I guess the situation you suggested above
won’t happen in the future.

How poor I am... : (
It’s just a simple program but you still help me find out a dozen of bugs.

Thank you DanKegel!