Use of the inhsocket option with nullmodem causes DOSBox to crash when it closes or when disabling the serial port

Are you using the latest Dosbox-Staging Version?

I have checked releases and am using the latest release.

Different version than latest?

No response

What Operating System are you using?

Windows 10

If Other OS, please describe

No response

Relevant hardware info

Intel i7 4790k, 32GB RAM, NVidia 2070 Super

Have you checked that no other similar issue already exists?

I have searched and not found similar issues.

A clear and concise description of what the bug is.

When using the inhsocket option with the serial nullmodem it causes DOSBox to crash when closing regardless of how you close it. If you try and disable the serial port using, for example, "serial1 disabled" the same crash occurs. This occurs when using either a TCP or UDP parent socket. I've even tried duplicating the socket and passing the duplicated socket number and it still crashes.

I noticed this with version 0.80.1 and went on to test 0.81.2 and 0.82.0. This does not occur with DOSBox SVN r4336 (the revision I'm using for the Win9x build of my project) under Win9x or Windows 10.

Aside from this crash, the inhsocket feature works perfectly.

Steps to reproduce the behaviour.

Explain how to reproduce

Create a UDP or TCP socket from a parent program.
Have the parent program spawn a child process to run DOSBox (instructing it to inherit handles) passing the socket number on the command line to dosbox.
inside dosbox setup a serial port to use inhsocket: "serial1 nullmodem inhsocket:1"
type, "exit" or "serial1 disabled" or just close the DOSBox window
crash.

Download URL of affected game or software

No response

Your configuration

No response

Provide a Log

Nothing is logged regarding why DOSBox crashed.

Code of Conduct & Contributing Guidelines

Yes, I agree.

Happens under 32 and 64-bit builds.

@jpetriejr

Thanks for the detailed report @jpetriejr and for the regression testing. Very well written 💯

Aside from this crash, the inhsocket feature works perfectly.

I'm glad to hear this is not a show stopper just a minor annoyance at exit.

@NicknineTheEagle You care about multiplayer & mode emulation, don't you? (and you're on Windows)

@NicknineTheEagle

No problem at all and thanks for the quick reply on this.

@NicknineTheEagle You care about multiplayer & mode emulation, don't you? (and you're on Windows)

Ha, you bet. I've implemented in my gaming service a means to link DOS games running natively under Win9x that use serial null modem to DOSBox instances over the Internet and the socket inheritance feature of DOSBox allowed for making it very efficient.

This looks like a null-modem option, I don't believe I've ever touched that. ^_^' I suppose you would need to look into CNullModem destructor.

This looks like a null-modem option, I don't believe I've ever touched that. ^_^' I suppose you would need to look into CNullModem destructor.

Yeah. I'll leave it to someone into this stuff, I don't believe it's widely used and I've never touched it. Plus it's just a shutdown annoyance.

I need some clarification on the reproduction steps, ideally with a minimally reproducible program that creates the socket and spawns the child.

I took a look at the code and just in case you weren't aware, the socket parameter it takes is actually a file descriptor. The first thing is does is call getpeername() with the command line socket argument as its first parameter.

I tried to reproduce this myself but it seems you also need to do more than simply create the socket. The socket needs to actually be connected to something or getpeername() returns an error and the connection fails (gracefully in my case, I can't reproduce the crash).

This is my quick and dirty socket + launch DOSBox code but it's Linux specifc. I need to know what exactly you're doing to trigger this.

#include <errno.h>
#include <stdio.h>
#include <string.h>
#include <sys/socket.h>
#include <unistd.h>

int main(void)
{
    int fd = socket(AF_INET, SOCK_STREAM, 0);
    if (fd == -1) {
        fprintf(stderr, "Failed to create socket: %s\n", strerror(errno));
    }
    char sock_str[32];
    sprintf(sock_str, "%d", fd);
    const char *path = "/home/daniel/code/dosbox-staging/build/debug/dosbox";
    if (execl(path, path, "--socket", sock_str, (char*)NULL) == -1) {
        fprintf(stderr, "Failed to launch DOSBox: %s\n", strerror(errno));
    }
    return 0;
}

Yes, I know it's a File Descriptor. And yes, the socket has to be connected to something. I stated that the inherited socket functionality is working, but for whatever reason ... very likely something in the null modem destructor is causing DOSBox to crash.

Under Windows the child process that is spawned has to be instructed to inherit handles so the socket number can be passed to DOSBox and it actually work. You would use CreateProcess() to do this.

I can provide a Windows binary I put together to test with when I first was looking into using the inhsocket option, but had tested with DOSBox SVN and didn't discover DOSBox Staging had an issue until I implemented everything in my client software. It's possible this may only be occurring under the Windows builds of DOSBox Staging.

I can test it under Windows if you give me something to test with. I'd prefer source code over a binary so I can fully understand what is happening.

My test program is written in Delphi 10.3 so you likely won't be able to use the source from it.

The source in C/C++ would essentially be the same thing as what you wrote above, but you have to use CreateProcess() to spawn the child process under windows. The issue though is when you close DOSBox or simply try and disable the serial port using something like "serial1 disabled". This is only happening under DOSBox Staging. mainline DOSBox is fine.

But, give me a few minutes and I'll post my test application.

haha ok I don't know Delphi. What are you connecting the socket to though? I think that's the main thing I'm missing. I'm not too sure what the usecase of this feature is.

The normal usage is that the socket is connected to my real relay server which uses UDP.

The test application is setting up a UDP socket which doesn't technically have to "connect" to anything since it's connectionless, but I need to add some input fields to allow you to set a host and port value in since I just had some hard coded values while I was testing and switching between TCP and UDP to see if there was a difference between them. There wasn't.

With UDP it's a easier to test since you don't have to worry about it connecting to anything. The socket opens regardless and DB Staging will present the same crash as when it's running within my real software whereas DB SVN does not in both cases.

Test.zip

Just put the full path to the DOSBox exe in the first edit box and click start. It will create the socket, launch DB passing the socket number and you can enter the command "serial1 nullmodem inhsocket:1" and close DB and it should crash.

Went ahead and ran it using DB Staging just to show the crash when attempting to exit.

The test application is setting up a UDP socket which doesn't technically have to "connect" to anything since it's connectionless

I only see code in Staging for TCP when using inhsocket so I don't think that's a good test.

dosbox-staging/src/hardware/serialport/nullmodem.cpp

Lines 88 to 116 in ad84f6f

    
           	// socket inheritance (client-alike) 
        
           	if (getUintFromString("inhsocket:", bool_temp, cmd)) { 
        
           #ifdef NATIVESOCKETS 
        
           		if (bool_temp == 1) { 
        
           			const auto arguments = &control->arguments; 
        
           			if (arguments->socket) { 
        
           				int sock    = *arguments->socket; 
        
           				dtrrespect  = false; 
        
           				transparent = true; 
        
           				LOG_MSG("SERIAL: Port %" PRIu8 " inheritance " 
        
           				        "socket handle: %d", 
        
           				        GetPortNumber(), sock); 
        
           				if (!ClientConnect(new TCPClientSocket(sock))) 
        
           					return; 
        
           			} else { 
        
           				LOG_MSG("SERIAL: Port %" PRIu8 " missing " 
        
           				        "\"-socket\" parameter.", 
        
           				        GetPortNumber()); 
        
           				return; 
        
           			} 
        
           		} else { 
        
           			LOG_MSG("SERIAL: Port %" PRIu8 " socket inheritance not " 
        
           			        "supported on this platform.", 
        
           			        GetPortNumber()); 
        
           			return; 
        
           		} 
        
           #else 
        
           		LOG_MSG("SERIAL: Port %" PRIu8 " socket inheritance not available.", 
        
           		        GetPortNumber());

So if you pass it a UDP socket, I'm not surprised if things blow up 😄

The interesting this is that the SVN codebase is exactly the same:

if (getBituSubstring("inhsocket:", &bool_temp, cmd)) {
#ifdef NATIVESOCKETS
		if (Netwrapper_GetCapabilities()&NETWRAPPER_TCP_NATIVESOCKET) {
			if (bool_temp==1) {
				int sock;
				if (control->cmdline->FindInt("-socket",sock,true)) {
					dtrrespect=false;
					transparent=true;
					LOG_MSG("Inheritance socket handle: %d",sock);
					if (!ClientConnect(new TCPClientSocket(sock)))
						return;
				} else {
					LOG_MSG("Serial%d: -socket parameter missing.",COMNUMBER);
					return;
				}
			}
		} else {
			LOG_MSG("Serial%d: socket inheritance not supported on this platform.",
				COMNUMBER);
			return;
		}
#else
		LOG_MSG("Serial%d: socket inheritance not available.", COMNUMBER);
#endif

But I don't think that matters since the socket code works regardless of what it was initialized as. Since it's just reading and writing to the socket. And like I said, it works just fine in usage. It just appears something is not being cleaned up properly when destroying the null modem object. Also, keep in mind that DB Staging also has the "sock" option you can also pass to the nullmodem which tells it if it's using TCP or UDP and I tried using that option and it doesn't affect the outcome.

Actually this looks like a use after free/double free bug. In the constructor for TCPClientSocket:

dosbox-staging/src/hardware/serialport/misc_util.cpp

Lines 551 to 552 in ad84f6f

    
           nativetcpstruct = new _TCPsocketX; 
        
           mysock = (TCPsocket)nativetcpstruct;

Then in the destructor:

dosbox-staging/src/hardware/serialport/misc_util.cpp

Lines 622 to 635 in ad84f6f

    
           TCPClientSocket::~TCPClientSocket() 
        
           { 
        
           #ifdef NATIVESOCKETS 
        
           	delete nativetcpstruct; 
        
           #endif 
        
           	if(mysock) { 
        
           		if(listensocketset) 
        
           			SDLNet_TCP_DelSocket(listensocketset, mysock); 
        
           		SDLNet_TCP_Close(mysock); 
        
           		LOG_INFO("SDLNET: Closed client TCP listening socket"); 
        
           	} 
        
           	if(listensocketset) SDLNet_FreeSocketSet(listensocketset); 
        
           }

I think mysock == nativetcpstruct. Can you try removing that delete nativetcpstruct; and see if it fixes it?

Man, I don't have the DOSBox Staging source downloaded or setup to debug or fix. But here is the constructor/destructor from DB SVN:

#ifdef NATIVESOCKETS
TCPClientSocket::TCPClientSocket(int platformsocket) {
	sendbuffer=0;
	nativetcpstruct = new Bit8u[sizeof(struct _TCPsocketX)];
	
	mysock = (TCPsocket)nativetcpstruct;
	isopen = false;
	if(!SDLNetInited) {
        if(SDLNet_Init()==-1) {
			LOG_MSG("SDLNet_Init failed: %s\n", SDLNet_GetError());
			return;
		}
		SDLNetInited = true;
	}
	// fill the SDL socket manually
	((struct _TCPsocketX*)nativetcpstruct)->ready=0;
	((struct _TCPsocketX*)nativetcpstruct)->sflag=0;
	((struct _TCPsocketX*)nativetcpstruct)->channel=(SOCKET) platformsocket;
	sockaddr_in		sa;
#ifdef OS2
	int			sz;
#else
	socklen_t		sz;
#endif
	sz=sizeof(sa);
	if(getpeername(platformsocket, (sockaddr *)(&sa), &sz)==0) {
		((struct _TCPsocketX*)nativetcpstruct)->
			remoteAddress.host=/*ntohl(*/sa.sin_addr.s_addr;//);
		((struct _TCPsocketX*)nativetcpstruct)->
			remoteAddress.port=/*ntohs(*/sa.sin_port;//);
	}
	else {
		mysock=0;
		return;
	}
	sz=sizeof(sa);
	if(getsockname(platformsocket, (sockaddr *)(&sa), &sz)==0) {
		((struct _TCPsocketX*)nativetcpstruct)->
			localAddress.host=/*ntohl(*/sa.sin_addr.s_addr;//);
		((struct _TCPsocketX*)nativetcpstruct)->
			localAddress.port=/*ntohs(*/sa.sin_port;//);
	}
	else {
		mysock=0;
		return;
	}
	if(mysock!=0) {
		listensocketset = SDLNet_AllocSocketSet(1);
		if(!listensocketset) return;
		SDLNet_TCP_AddSocket(listensocketset, mysock);
		isopen=true;
		return;
	}
	mysock=0;
	return;
}
#endif // NATIVESOCKETS

TCPClientSocket::~TCPClientSocket() {
	
	if(sendbuffer) delete [] sendbuffer;
#ifdef NATIVESOCKETS
	if(nativetcpstruct) delete [] nativetcpstruct;
	else
#endif
	if(mysock) {
		if(listensocketset) SDLNet_TCP_DelSocket(listensocketset,mysock);
		SDLNet_TCP_Close(mysock);
	}

	if(listensocketset) SDLNet_FreeSocketSet(listensocketset);
}

They have an else after they delete nativetcpstruct so it doesn't double free. I'm betting that's the bug. I'll test on Windows later to confirm.

Yep, good catch!

And thanks for taking the time to look into this. I greatly appreciate it.

@jpetriejr

Thanks for the test program @jpetriejr

That reproduced the bug for me nicely on Windows and I put a PR in with the fix.

Beautiful! Thank you sir.

Navigation Menu

Repository navigation

Use of the inhsocket option with nullmodem causes DOSBox to crash when it closes or when disabling the serial port #4112

Are you using the latest Dosbox-Staging Version?

Different version than latest?

What Operating System are you using?

If Other OS, please describe

Relevant hardware info

Have you checked that no other similar issue already exists?

A clear and concise description of what the bug is.

Steps to reproduce the behaviour.

Download URL of affected game or software

Your configuration

Provide a Log

Code of Conduct & Contributing Guidelines

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	// socket inheritance (client-alike)
	if (getUintFromString("inhsocket:", bool_temp, cmd)) {
	#ifdef NATIVESOCKETS
	if (bool_temp == 1) {
	const auto arguments = &control->arguments;
	if (arguments->socket) {
	int sock = *arguments->socket;
	dtrrespect = false;
	transparent = true;
	LOG_MSG("SERIAL: Port %" PRIu8 " inheritance "
	"socket handle: %d",
	GetPortNumber(), sock);
	if (!ClientConnect(new TCPClientSocket(sock)))
	return;
	} else {
	LOG_MSG("SERIAL: Port %" PRIu8 " missing "
	"\"-socket\" parameter.",
	GetPortNumber());
	return;
	}
	} else {
	LOG_MSG("SERIAL: Port %" PRIu8 " socket inheritance not "
	"supported on this platform.",
	GetPortNumber());
	return;
	}
	#else
	LOG_MSG("SERIAL: Port %" PRIu8 " socket inheritance not available.",
	GetPortNumber());

	nativetcpstruct = new _TCPsocketX;
	mysock = (TCPsocket)nativetcpstruct;

	TCPClientSocket::~TCPClientSocket()
	{
	#ifdef NATIVESOCKETS
	delete nativetcpstruct;
	#endif
	if(mysock) {
	if(listensocketset)
	SDLNet_TCP_DelSocket(listensocketset, mysock);
	SDLNet_TCP_Close(mysock);
	LOG_INFO("SDLNET: Closed client TCP listening socket");
	}

	if(listensocketset) SDLNet_FreeSocketSet(listensocketset);
	}

Use of the inhsocket option with nullmodem causes DOSBox to crash when it closes or when disabling the serial port #4112

Description

Are you using the latest Dosbox-Staging Version?

Different version than latest?

What Operating System are you using?

If Other OS, please describe

Relevant hardware info

Have you checked that no other similar issue already exists?

A clear and concise description of what the bug is.

Steps to reproduce the behaviour.

Download URL of affected game or software

Your configuration

Provide a Log

Code of Conduct & Contributing Guidelines

Activity

jpetriejr commented on Dec 18, 2024

johnnovak commented on Dec 19, 2024

jpetriejr commented on Dec 19, 2024

NicknineTheEagle commented on Dec 19, 2024

johnnovak commented on Dec 19, 2024

weirddan455 commented on Dec 20, 2024

jpetriejr commented on Dec 20, 2024

weirddan455 commented on Dec 20, 2024

jpetriejr commented on Dec 20, 2024

weirddan455 commented on Dec 20, 2024

jpetriejr commented on Dec 20, 2024

jpetriejr commented on Dec 20, 2024

jpetriejr commented on Dec 20, 2024

jpetriejr commented on Dec 20, 2024

weirddan455 commented on Dec 20, 2024

jpetriejr commented on Dec 20, 2024

weirddan455 commented on Dec 20, 2024

jpetriejr commented on Dec 20, 2024

weirddan455 commented on Dec 20, 2024

jpetriejr commented on Dec 20, 2024

weirddan455 commented on Dec 20, 2024

jpetriejr commented on Dec 20, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions