Null pointer dereferences

Developer's Forum, for discussion of bugs, code, and other developmental aspects of DOSBox.

Null pointer dereferences

Postby realnc » 2019-11-06 @ 15:21

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

Code: Select all
$ ./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:

Code: Select all
//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?
User avatar
realnc
Member
 
Posts: 439
Joined: 2010-10-13 @ 11:02

Re: Null pointer dereferences

Postby jmarsh » 2019-11-06 @ 19:38

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".
jmarsh
Member
 
Posts: 317
Joined: 2014-1-04 @ 09:17

Re: Null pointer dereferences

Postby realnc » 2019-11-06 @ 19:59

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.
User avatar
realnc
Member
 
Posts: 439
Joined: 2010-10-13 @ 11:02

Re: Null pointer dereferences

Postby jmarsh » 2019-11-06 @ 20:20

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

Under what situation do you imagine it returning random garbage?
jmarsh
Member
 
Posts: 317
Joined: 2014-1-04 @ 09:17

Re: Null pointer dereferences

Postby realnc » 2019-11-06 @ 20:28

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.)
User avatar
realnc
Member
 
Posts: 439
Joined: 2010-10-13 @ 11:02

Re: Null pointer dereferences

Postby jmarsh » 2019-11-06 @ 20:43

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.
jmarsh
Member
 
Posts: 317
Joined: 2014-1-04 @ 09:17

Re: Null pointer dereferences

Postby dreamer_ » 2019-11-08 @ 11:49

@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-stagi ... 921720b8d6 - 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
Attachments
0001-Avoid-potential-undefined-behaviour.patch
(2.87 KiB) Downloaded 4 times
Code: Select all
| ← Ceci n'est pas une pipe
User avatar
dreamer_
Newbie
 
Posts: 43
Joined: 2019-5-17 @ 20:19

Re: Null pointer dereferences

Postby realnc » 2019-11-08 @ 12:11

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.
User avatar
realnc
Member
 
Posts: 439
Joined: 2010-10-13 @ 11:02

Re: Null pointer dereferences

Postby DosFreak » 2019-11-08 @ 12:41

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 viewtopic.php?f=31&t=55706&p=792269&hilit=compilation#p792237

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.
User avatar
DosFreak
l33t++
 
Posts: 10497
Joined: 2002-6-30 @ 16:35
Location: Your Head

Re: Null pointer dereferences

Postby krcroft » 2019-11-08 @ 23:22

realnc wrote:There's a few places in the dosbox code where sanitizers detect null pointer derefs at runtime. Example:
Code: Select all
$ ./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.
User avatar
krcroft
Member
 
Posts: 404
Joined: 2017-4-29 @ 15:07
Location: Ogden's Retreat

Re: Null pointer dereferences

Postby dreamer_ » 2019-11-09 @ 10:17

@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.
Code: Select all
| ← Ceci n'est pas une pipe
User avatar
dreamer_
Newbie
 
Posts: 43
Joined: 2019-5-17 @ 20:19

Re: Null pointer dereferences

Postby realnc » 2019-11-09 @ 10:39

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.
User avatar
realnc
Member
 
Posts: 439
Joined: 2010-10-13 @ 11:02

Re: Null pointer dereferences

Postby krcroft » 2019-11-09 @ 16:23

@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):
Code: Select all
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-stagi ... =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 :-)
Attachments
clang-msan.zst.txt
(410.25 KiB) Not downloaded yet
clang-usan.log.gz
(1.81 KiB) Not downloaded yet
gcc-usan.log.gz
(2.4 KiB) Not downloaded yet
gcc-uasan.log.gz
(3.68 KiB) Not downloaded yet
gcc-tsan.log.gz
(1.53 KiB) Not downloaded yet
User avatar
krcroft
Member
 
Posts: 404
Joined: 2017-4-29 @ 15:07
Location: Ogden's Retreat

Re: Null pointer dereferences

Postby dreamer_ » 2019-11-12 @ 09:53

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-stagi ... 21d8bf6554
Code: Select all
| ← Ceci n'est pas une pipe
User avatar
dreamer_
Newbie
 
Posts: 43
Joined: 2019-5-17 @ 20:19

Re: Null pointer dereferences

Postby Qbix » 2019-11-12 @ 12:26

Thanks for spotting it. Should be fixed in 4283.
Water flows down the stream
How to ask questions the smart way!
User avatar
Qbix
DOSBox Author
 
Posts: 10947
Joined: 2002-11-27 @ 14:50
Location: Fryslan

Re: Null pointer dereferences

Postby dreamer_ » 2019-11-12 @ 14:51

r4283 definitely fixes the problem, thanks :)
Code: Select all
| ← Ceci n'est pas une pipe
User avatar
dreamer_
Newbie
 
Posts: 43
Joined: 2019-5-17 @ 20:19


Return to DOSBox Development

Who is online

Users browsing this forum: No registered users and 3 guests