VOGONS

Null pointer dereferences

Topic actions

First post, by realnc

Posted on 
User metadata
Rank Oldbie
Rank
Oldbie

There's a few places in the dosbox code where sanitizers detect null pointer derefs at runtime. Example:

$ ./dosbox
...
dbopl.cpp:1447:47: runtime error: member access within null pointer of type 'DBOPL::Chip'
dbopl.cpp:1462:47: runtime error: member access within null pointer of type 'DBOPL::Channel'
...

The code is:

//Create a table with offsets of the channels from the start of the chip
DBOPL::Chip* chip = 0;
for ( Bitu i = 0; i < 32; i++ ) {
	Bitu index = i & 0xf;
	if ( index >= 9 ) {
		ChanOffsetTable[i] = 0;
		continue;
	}
	//Make sure the four op channels follow eachother
	if ( index < 6 ) {
		index = (index % 3) * 2 + ( index / 3 );
	}
	//Add back the bits for highest ones
	if ( i >= 16 )
		index += 9;
	Bitu blah = reinterpret_cast<Bitu>( &(chip->chan[ index ]) );  <====== *** HERE ***
	ChanOffsetTable[i] = blah;
}

What's the intent here? Or are these just plain old bugs?

Reply 1 of 15, by jmarsh

Posted on 
User metadata
Rank Oldbie
Rank
Oldbie

It looks like old code written to be portable before "offsetof" was widely available.
It's not dereferencing a NULL pointer, it's getting the address of a member variable when the "this" pointer is NULL which is equivalent to finding the offset of that member (in bytes) from "this".

Reply 2 of 15, by realnc

Posted on 
User metadata
Rank Oldbie
Rank
Oldbie
jmarsh wrote:

It looks like old code written to be portable before "offsetof" was widely available.
It's not dereferencing a NULL pointer, it's getting the address of a member variable when the "this" pointer is NULL which is equivalent to finding the offset of that member (in bytes) from "this".

That's undefined behavior though. It might get the offset, it might get random garbage.

Reply 3 of 15, by jmarsh

Posted on 
User metadata
Rank Oldbie
Rank
Oldbie

No it's not. Members are always at a fixed location within classes.

Under what situation do you imagine it returning random garbage?

Reply 4 of 15, by realnc

Posted on 
User metadata
Rank Oldbie
Rank
Oldbie
jmarsh wrote:

No it's not. Members are always at a fixed location within classes.

Under what situation do you imagine it returning random garbage?

When the compiler assumes that 'chip' isn't null, because that would be UB and the compiler is free to assume that this can't happen.

Also, the clang ubsan is not expected to be producing false positives (it's extremely rare.) This looks like UB to me, it just happens that most compilers will produce the expected result (which is a valid outcome with UB.)

Reply 5 of 15, by jmarsh

Posted on 
User metadata
Rank Oldbie
Rank
Oldbie

It's only undefined behaviour if the pointer is dereferenced. That's not happening here.

Look up the definition of offsetof and see what it does.

Reply 6 of 15, by dreamer_

Posted on 
User metadata
Rank Member
Rank
Member

@jmarsh, thank you for explaining what's happening here and referencing `offsetof`. You are right, there is no UB here… as long as quiet assumption about underlying type using standard layout holds true… and when high-quality C++ compiler is being used.

@realnc - thanks for starting this thread, I think your concern in here is valid. This piece od code is not easy to understand and quite brittle. I implemented a fix here: https://github.com/dreamer/dosbox-staging/com … aef8d921720b8d6 - check it out and let me know if it prevents sanitizer warnings. (I attached a patch as well).

Feel free to leave comments in review for this change: https://github.com/dreamer/dosbox-staging/pull/37

There's also an open review for introducing sanitizer checks to dosbox-staging CI: https://github.com/dreamer/dosbox-staging/pull/33

| ← Ceci n'est pas une pipe
dosbox-staging

Reply 7 of 15, by realnc

Posted on 
User metadata
Rank Oldbie
Rank
Oldbie

Well, it looked like UB, but jmarsh seems more knowledgeable about the matter. I thought that anything that derefs a null pointer in an evaluated context is considered UB by the C++ standard, even if the deref isn't actually happening. But I'm no language lawyer.

In any event, I'm not sure anything higher than C++98 is being accepted in dosbox. I've seen commits in SVN that remove C++11 code that snuck in. I'm not sure the patch will be accepted.

Reply 8 of 15, by DosFreak

Posted on 
User metadata
Rank l33t++
Rank
l33t++

C++98 for GCC
C++03 for VS

C++11 would probably bump the minimum VS requirement from VS 2003 to VS2012 or VS2013 and mabye GCC 4.8.1.

No loss of Windows OS compatibility when compiled with mingw w/ GCC but VS would be XP minimum due to the CRT breaking compatibility. Although I'm looking into that Post 792269

When DOSBox switches to SDL2 then it may make sense to make c++11 the minimum and as long as DOSBox is compiled with mingw w/gcc without SSE2 support then DOSBox should still work on 98/ME (kernelex) or (2000) with BlackwingCat Extended Kernel or Unofficial SDL 2.0.5 for 2000 on the same hardware it does now.

How To Ask Questions The Smart Way
Make your games work offline

Reply 9 of 15, by krcroft

Posted on 
User metadata
Rank Oldbie
Rank
Oldbie
realnc wrote:
There's a few places in the dosbox code where sanitizers detect null pointer derefs at runtime. Example: […]
Show full quote

There's a few places in the dosbox code where sanitizers detect null pointer derefs at runtime. Example:

$ ./dosbox
dbopl.cpp:1447:47: runtime error: member access within null pointer of type 'DBOPL::Chip'
dbopl.cpp:1462:47: runtime error: member access within null pointer of type 'DBOPL::Channel'

@realnc, can you share which compiler and sanitizer flags you were using? Also, do you need a specific dosbox config (op2, opl3, compat, fast, etc..) to drive the code to this path?

I tried a couple runs today with some sanitizers enabled, oplmode=auto and oplemu=compat, and couldn't reproduce these OLP-specific ones (plenty of others though) with a couple FM-synth games. I suspect I'm not using the right combination of flags.

Reply 10 of 15, by dreamer_

Posted on 
User metadata
Rank Member
Rank
Member

@krcroft I haven't built the code with sanitizers when developing this patch, but I managed to hit the codepath just by starting a dosbox (without any autoexec section). I had `oplmode=auto` and `oplemu=default` in `sblaster` section.

| ← Ceci n'est pas une pipe
dosbox-staging

Reply 11 of 15, by realnc

Posted on 
User metadata
Rank Oldbie
Rank
Oldbie
krcroft wrote:

@realnc, can you share which compiler and sanitizer flags you were using? Also, do you need a specific dosbox config (op2, opl3, compat, fast, etc..) to drive the code to this path?

I only used "-fsanitize=undefined". No specific config needed. I used both Clang 9.0.0 and GCC 9.2.0 and both find the same issues.

Reply 12 of 15, by krcroft

Posted on 
User metadata
Rank Oldbie
Rank
Oldbie

@realnc, @dreamer - thank you! Managed to hit it now.

I've differentiated the various sanitizers and tested each (attached):
- usan: detect undefined behavior and integer overflows (working on clang and gcc)
- asan: detect out of bounds use, double free, and leaks (broken on clang and gcc)
- msan: detect uninitialized reads and use-after-destruction (only available for clang)
- uasan: detect both undefined (usan) and address (asan) issues (broken on clang)
- tsan: detect data-race threading bugs (only available for gcc)

The broken ones fail to make it through ./configure because they detect a flaw within the micro-builds performed by autotools. So I might need to find a way around those.
Perhaps explicitly passing {C,CXX,LD}FLAGS into make directly and only feeding ./configure safe flags.

The memory sanitizer run log is 1GB, before I gave up. Fortunately it packs down to 400KB using zstd. You can read it using zstd's on-the-fly decompressor (no need to extract 1GB):

zstdless --long=30 clang-msan.zst.txt

Over at dosbox-staging, the analyzers are planned to run as part of the CI workflow.
You can se live output from each sanitizer here, the output is zipped and available in the top right ''Artifacts' pulldown (created when all the runs are complete):
https://github.com/dreamer/dosbox-staging/com … te_id=303404134

Currently we have one testcase for the analyzers: "Open DOSBox and then quit".
Fortunately there's enough reported to keep our boots filled for a while before we need to start exercising more code paths 😀

Reply 13 of 15, by dreamer_

Posted on 
User metadata
Rank Member
Rank
Member

A change for this issue was committed to svn/trunk@4282. I strongly suspect it introduces a regression though, as macros REGOP and REGCHAN depend in runtime on the value stored in offset tables being >0 to trigger code. Not sure which games are going to see the regression…

I won't bother with uploading a patch, as I know it will be rejected, but most likely dosbox-staging will have r4282 reverted and my fix included instead. In future I might make this code back in sync with SVN, depending on if and how this will be resolved.

I added more asserts to prevent this bug from occurring: https://github.com/dreamer/dosbox-staging/pull/37 specifically this commit: https://github.com/dreamer/dosbox-staging/com … 4828821d8bf6554

| ← Ceci n'est pas une pipe
dosbox-staging

Reply 14 of 15, by Qbix

Posted on 
User metadata
Rank DOSBox Author
Rank
DOSBox Author

Thanks for spotting it. Should be fixed in 4283.

Water flows down the stream
How to ask questions the smart way!

Reply 15 of 15, by dreamer_

Posted on 
User metadata
Rank Member
Rank
Member

r4283 definitely fixes the problem, thanks 😀

| ← Ceci n'est pas une pipe
dosbox-staging

Go to top of page Go to top of page
Back to DOSBox Development
Close window
Close window