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 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.

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

Under what situation do you imagine it returning random garbage?

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.)

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, 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

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.

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.

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:

Copy code to clipboard

1$ ./dosbox
2dbopl.cpp:1447:47: runtime error: member access within null pointer of type 'DBOPL::Chip'
3dbopl.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.

@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.

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.

@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):

1zstdless --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 😀

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

Thanks for spotting it. Should be fixed in 4283.

r4283 definitely fixes the problem, thanks 😀