Assess and fix Coverity issues prior to 0.81 release

[kcgen]

These are all very minor and likely need just a line or two to fix up.

Feel free to grab any of them and push single fix up PRs.

1. 

2. 

3. 

4. 

5. 

[weirddan455]

Some of these are from third party libraries. I saw at least SDL Sound and stb_vorbis in there. Do we really want to be diverging from upstream for some minor warning fixes? Or should be ignoring these warnings from the libs directory?

[kcgen]

Yup, PR or report as much as possible upstream (just like you did w/ libmt32emu). For STB Vorbis specifically, we've:

• reported 19 issues: https://github.com/nothings/stb/issues?q=author%3Akcgen+is%3Aclosed -
• PR'd fixed for two Coverity issues: STB Vorbis: Coverity static-analysis scan fixes nothings/stb#854
• PR'd fixes for file handling issues: Add detection for Ogg skeleton metadata nothings/stb#678

Suggest a similar approach here; and no problem carrying this issue in Coverity as long as needed till upstream fixes it.

We've had a similar approach for other ported-in sources too (like Enet, dr_flac, dr_wav, dr_mp3, PD curses, slirp, .. and maybe others).

[kcgen]

For SDL_Sound.c, this is carried over from SDL1 (when SDL2 had abandoned it).

It's highly cut down just for our needs for compressed CDDA audio (but still has the same original API).

All that to say, it's own own file to mangle and maintain for now (it's received lots of prior PVS and Coverity fixes, as well as warning cleanups).

I see SDL has kind of brought back SDL2_Sound to life.. so maybe we can externalize it and toss this file down the road.

[kcgen]

@weirddan455 , I've sent an invite from the (clunky) Coverity system. Should appear in your email.

When you visit, there should be a grey button "Login with GitHub", should get you in without any passwords or account details needed.

https://scan.coverity.com/projects/dosbox-staging

• The main page has metrics and summaries over time.

• Use the right-hand "View Defects" button to get to the issues.

  

Once in the defect dashboard, use the burger in the upper-left and select "all in project":

And from there, click on one of the outstanding quantities:

[kcgen]

(@johnnovak - oh, I see you're not in the list too; invite sent)

[kcgen]

Coverity maps each issue to a Common Weakness Enumeration: CWE, which is automatic justification for the fix 😄 !

You can see prior fixes with: git log --oneline | grep '(CWE.*)'

03d23c921 Cleanup an unintended sign extension warning in voodoo (CWE-194)
8cf51acc6 Fix a use-after-move in the string_format routine (CWE-457)
30c7b5b1b Fix an uninitialized scalar variable warning in rwqueue (CWE-457)
cdd1d9920 Avoid a sign extension when computing the image size (CWE-194)
c2d31520e Check return code when deflating ZMBV frames (CWE-252)
6f5b83872 Check return code when imgmount'ing boot images (CWE-252)
45853f7d4 Check return code from volume label lookup (CWE-252)
4f4b3ff7f Check a return value in shell::which (CWE-252)
6259bf145 Fix a Coverity warning in Drive FAT (CWE-369)
d3bd339a1 Check the return code when looking up batch env variables (CWE-252)
9d0be093e Assert a loop bound will never be exceeded in IMFC (CWE-125)
06398d2ac Normalize a loop condition in IMFC (CWE-835)
ccc5bedab Fix an unchecked dynamic cast in DOS Devices (CWE-476)
1843b4bda Avoid use an uninitialized scalar in PC Speaker (CWE-457)
e0761e7e1 Fix a time-of-check time-of-use (TOCTOU) scenario in cross utils (CWE-367)
9cbae602a Avoid reading uninitialized memory (CWE-457)
3305af832 Check seek result when reading language codes from layout file (CWE-252)
aef8d8061 Check SERIAL.COM's port-type argument (CWE-252)
4a6387c7e Avoid shifting negative DAC data in Sound Blaster #2113 (CWE-758)
dcd835d8d Avoid an unnecessary pointer check in V-File (CWE-476)
2b12bb4a0 Free virtual files' data and name on removal (CWE-401)
4149f179b Avoid a double-free when closing Opus tracks (CWE-415)
ea919bd88 Refactor ReelMagic player with per-stream handle registration (CWE-119)
57087ab2c Fix an unintended sign extension in the ReelMagic video mixer (CWE-194)
3419eaa9f Avoid a file-size null dereference in the ReelMagic driver (CWE-476)
c12c2f2de Avoid an out-of-bounds write in the ReelMagic MPEG player (CWE-119)
09eb20e0a Use a static-assert to confirm callback size limits (CWE-569)
1c364a98d Check file seek when MPEG decoder seeks in its buffer (CWE-252)
b632b6eab Check file seek when MPEG decoder creates a buffer (CWE-252)
33bef71d7 Defer header inspection from MPEG decoder's muxer creation (CWE-252)
bc3ad6e30 Defer decoder creation from MPEG decoder's buffer creation (CWE-252)
41498dc45 Fix a time-of-check time-of-use (TOCTOU) scenario in ManyMouse (CWE-367)
bf183f21d Fix uninitialized event member access in ManyMouse (CWE-457)
1e47dc45d Null-terminate the boot ROM buffer after reading (CWE-170)
6ace74946 Initialize the impulse PC Speaker PIT times from a constexpr (CWE-457)
23ff06c53 Check if an argument was provided in the serial command (CWE-252)
c8a3168b8 Check for an out-of-bounds read condition in SB DMA (CWE-125)
05ee3fbfd Check for an out-of-bounds read condition in OPL (CWE-125)
8369a1838 Use the Mersenne Twister random generator in TalChorus (CWE-676)
970520dbc Allow exceptions to be thrown from Adlib Golb (CWE-248)
63d392061 Avoid out-of-bounds access on large SB DMA transfers (CWE-125)
921885d09 Use size-limited string operations in drive functions (CWE-120)
903c47d28 Initialize the CDROM read-sectors buffer before use (CWE-457)
3bf528c33 Assert type-bounds before value-bounds in check_cast (CWE-569)
9c69fbdef Simplify bitops width checks (CWE-569)
2cc219c7d Avoid uncaught exception during setup destruction (CWE-248)
8a3bc19e3 Avoid a buffer overrun when splitting 8.3 filenames (CWE-120)
48449e152 Initialize the impulse PC Speaker max PIT rate (CWE-457)
250e31f46 Allow an exception from Adlib Gold's lowshelf to be caught (CWE-248)

   ...

[weirddan455]

Thanks for the invite @kcgen I was able to get logged in. Working with upstream seems like a good policy then.

I see SDL has kind of brought back SDL2_Sound to life.. so maybe we can externalize it and toss this file down the road.

That is probably for the best. It would potentially remove a couple dependencies as well. I believe libvorbis and speex are only needed for SDL Sound?

https://github.com/icculus/SDL_sound

Current versions of SDL_sound do not use any external libraries for decoding various file formats. All needed decoder source code is included with the library

They've done away with those requirements for the SDL Sound 2.0 release.

Are we trying to release 0.81 in the near future though? If so, we may want to wait on updating that until post-release in case there's regressions. As far as I can tell, the SDL Sound stuff has "just worked" for us for a while now so if there's not much user facing benefit we can probably wait. Although I do think it would eliminate some tech debt to update maybe after 0.81.

[kcgen]

DOSBox Staging's SDL_Sound

Thanks for the invite @kcgen I was able to get logged in.

👍

It would potentially remove a couple dependencies as well. I believe libvorbis and speex are only needed for SDL Sound?

These aren't DOSBox Staging dependencies (we don't use either of them). Our SDL_Sound only has libopus as a dependency.

If you're seeing libvorbis and speex in ldd output, these are coming in as second-level dependencies, probably from SDL2 itself.

Working with upstream seems like a good policy then.

When I was working on the original CD-DA FLAC/Opus/MP3 patch for upstream DOSBox, I chatted w/ Ryan from SDL about including Opus in SDL_Sound and he was interested, however the new goal of minimizing dependencies was a priority and so Opus wasn't added.

David Reid (of dr_libs) does have an Opus request ticket and has been chipping away at realizing dr_opus.h: mackron/dr_libs#139. Surely when that's available, Ryan would be OK with adding that to SDL_Sound 2.0.

So that's a big functional gap; we can't moved until SDL_Sound 2 has Opus support.

A second difference is that our SDL_Sound set s up an MP3 fast seek table (and caches it to disk). Some games use a single giant audio track and seek within it for music and voice sequences. On slow(er) hardware, seeking to PCM-exact frames can be very slow.

Are we trying to release 0.81 in the near future though? If so, we may want to wait on updating that until post-release in case there's regressions. As far as I can tell, the SDL Sound stuff has "just worked" for us for a while now so if there's not much user facing benefit we can probably wait. Although I do think it would eliminate some tech debt to update maybe after 0.81.

Yes to all (in the immediate term), but also longer term I don't see up migrating until SDL2_Sound supports Opus and the MP3 fast seek tables.

But there's probably a hybrid approach that gets us closer (also, just a longer term goal):

We could fork SDL2_Sound into the dosbox-staging organization, then layer just our add-ons as a branch, and we could start to use this "layered-branch", which would let us easily track the upstream repo.

But yeah.. that's longer term.

[weirddan455]

These aren't DOSBox Staging dependencies (we don't use either of them). Our SDL_Sound only has libopus as a dependency.

If you're seeing libvorbis and speex in ldd output, these are coming in as second-level dependencies, probably from SDL2 itself.

Yeah, sorry it was libopus I was thinking of, not libvorbis. I guess we're using stb_vorbis which is in-tree. Speex is a dependency of something though, we even have a wrap for it.

dosbox-staging/meson.build

Lines 525 to 535 in 904cb68

	# SpeexDSP
	# ~~~~~~~~
	# Default to the system library
	speexdsp_dep = dependency(
	'speexdsp',
	version: ['>= 1.2', '< 2'],
	required: false,
	fallback: [],
	static: ('speexdsp' in static_libs_list or prefers_static_libs),
	include_type: 'system',
	)

But, OK point taken about SDL Sound. We have essentially our own fork of that then. I think you should leave that Coverity warning in the original post and we can potentially fix that ourselves (and not worry about upstream). It was marked a use after free bug so it could be something serious. I also saw the volitile keyword in there which is a red flag. Volitile is often incorrectly used for thread synchronization.

[shermp]

Isn't speexdsp used for audio resampling in DOSBox? I seem to recall that's the reason for it's inclusion.

[kcgen]

Yeah, sorry it was libopus I was thinking of, not libvorbis.

no worries; easy to mix them up (all the Xiph libs :-)

I guess we're using stb_vorbis which is in-tree.

Yup.

(@weirddan455 )

Speex is a dependency of something though, we even have a wrap for it.

(@shermp )

Isn't speexdsp used for audio resampling in DOSBox? I seem to recall that's the reason for it's inclusion.

Yup, Speex DSP is our resampler - it's confusing because it's an off-shoot from Xiph's compression codec (Speex), but just the DSP resampler (without the Speex codec).

It's super-efficient, small, and has a nice API.

[kcgen]

I think you should leave that Coverity warning in the original post and we can potentially fix that ourselves (and not worry about upstream). It was marked a use after free bug so it could be something serious. I also saw the volitile keyword in there which is a red flag. Volitile is often incorrectly used for thread synchronization.

Roger that; will re-add. It's actually a very stubborn issue in the code that I've tried to fix before. Having you help squash that would be a big help, @weirddan455 👍 !

[johnnovak]

Are we trying to release 0.81 in the near future though? If so, we may want to wait on updating that until post-release in case there's regressions.

Updating you here @weirddan455 because you rarely show up at our chat. Btw, will send you an invite to our private channel once you register a permanent Discord account.

The team has decided in the past few days that we'll switch to a release candidate style process. In addition to that, I'll be offline for three weeks from Wednesday and I have some outstanding stuff to finish, some of which will likely only happen when I'm back. This means that we can cut a release around early Dec in the ideal scenario, then release it this year after say 2 weeks of internal testing.

And yes, we need the resampler from Speex; the whole audio pipeline depends on it, that's something I introduced last year.

Thanks for the Coverity invite @kcgen.

[kcgen]

Thanks for the quick release plan here, @johnnovak -- relayed to @GranMinigun and @Grounded0.

[weirddan455]

This warning is an annoying micro-optimization:

dosbox-staging/src/gui/sdlmain.cpp

Lines 4673 to 4681 in 904cb68

	for (auto line : arguments->set) {
	trim(line);
	if (line.empty() || line[0] == '%' || line[0] == '\0' ||
	line[0] == '#' || line[0] == '\n') {
	continue;
	}
	std::vector<std::string> pvars(1, std::move(line));

There's nothing wrong with this code as far as I'm concerned. I believe I touched this code when I refactored command line argument parsing.

It's complaining that it's making a copy of a std::string but I explicitly wanted a copy here. trim modifies the string in place and then it gets std::move'd into another vector. Making it a reference like it wants likely leads to bugs down the road as it would mangle the original vector of arguments.

[weirddan455]

Raised an issue upstream with stb: nothings/stb#1528

[weirddan455]

I edited the OP to remove the issue's I've fixed. SDL Sound is still problematic.

I'm explicitly passing in sample_list as a parameter to Sound_FreeSample and added an assert in the branch where there is a non-null prev pointer saying "if you take this branch, you shouldn't be looking at sample_list (head of linked list). I also wrapped the entire loop in a mutex lock so no other thread should be able to modify the sample_list global.

But somehow Coverity saw my assert and decided that sample != sample_list. How? No one has changed it. @kcgen suggested to mark this as a false positive as he's run it through ASAN and not found any leaks or double frees. I'm not sure how to do that though.

[weirddan455]

As a side note, while I'm pretty sure this is going to be a false positive, we could avoid this issue by refactoring to replace the handrolled linked list with a std::vector (would also require changing from C to C++). Maybe even refactor the entire file to be honest. We've already significantly diverged from upstream and I'm not a big fan of carrying around legacy cruft if we have to maintain this ourselves.

Maybe I'll put this as a TODO for after 0.81.

[kcgen]

suggested to mark this as a false positive

👍 ; marked.

we could avoid this issue by refactoring to replace the handrolled linked list with a std::vector (would also require changing from C to C++). Maybe even refactor the entire file to be honest.

Yup; although this is surely a bug in Coverity, we can take its struggles as a sign that there's way too much opaque pointer casting stuff going on.

The API is perfect fit for a pure-virtual base class that avoids all the void-pointer casting internals, as well. I suspect a lot of internal code would disappear.

[kcgen]

I think this has been wrapped up; thanks for all the fixes, @weirddan455.