IBM Music Feature Card doesn't work with Silpheed

[FredBezies]

Describe the bug

Hello.

I tried to build dosbox-x from git commit 0d4f7ac and I got this error line.

imfc.cpp:99:15: error: format not a string literal and no format arguments [-Werror=format-security]
   99 |         printf((format + "\n").c_str(), args...);
      |         ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

gcc used: 13.1.1 on Archlinux.

Steps to reproduce the behaviour

Just try to build commit 0d4f7ac using gcc 13.1.1.

Expected behavior

Build process going on.

What operating system(s) this bug have occurred on?

Archlinux

What version(s) of DOSBox-X have this bug?

commit 0d4f7ac924971eb2b304c22f77feb1d3b45134f8

Used configuration

No response

Output log

No response

Additional information

No response

Have you checked that no similar bug report(s) exist?

• [X] I have searched and didn't find any similar bug report.

Code of Conduct & Contributing Guidelines

• [X] I agree to follow the code of conduct and the contributing guidelines.

[Allofich]

That IMF_LOG function doesn't even put anything out to the DOSBox-X log, so it's easiest to just comment out that IMF_LOG and make it non-functional. If we want the IMF_LOG functionality we can fix it later, but right now I doubt anyone cares. I just put up a PR.

@kcgen, this may be relevant for DOSBox Staging as well because that line is exactly the same in DOSBox Staging's imfc.cpp.

[kcgen]

Thanks is for the heads up @Allofich ; yup - I think Staging is skating by only because we're not building with gcc 13.x yet 😅. Will cherry-pick your fix when things break.

Indeed, the internal logging is for anyone working on IMFC emulation itself, and no sweat to comment that back in.

[grapeli]

Once "-Wformat -Werror=format-security" can be excluded from the build flags. Two, it's not only gcc 13 supports it, much older ones support it too. In this clang for a long time. Three is easy to fix. https://fedoraproject.org/wiki/Format-Security-FAQ

- printf((format + "\n").c_str(), args...);
+ printf((format + "\n").c_str(), "%s", args...);

[Allofich]

printf((format + "\n").c_str(), "%s", args...); may silence the error (see below), but IMF_LOG gets passed, for example, this string: %s: new mode: Group0(%s/%s/%s) / Group1(%s/%s/%s) with the values to substitute for each %s.

With the existing printf statement (the one with the compiler error), this gives an output like: PIU_IMF: new mode: Group0(MODE1/OUTPUT/OUTPUT) / Group1(MODE1/INPUT/OUTPUT)

But with printf((format + "\n").c_str(), "%s", args...); %s will be substituted for the first %s, then the other values will be substituted in, giving

%s: new mode: Group0(PIU_IMF/MODE1/OUTPUT) / Group1(OUTPUT/MODE1/INPUT))

Moving the %s first, like printf("%s", (format + "\n").c_str(), args...);, doesn't work either, that gives %s: new mode: Group0(%s/%s/%s) / Group1(%s/%s/%s)

Replacing the printf with a DOSBox-X logging function (a LOG call) might avoid the error, but the imfc code gives a bunch of instrument %i (midi channel %i) - mask 0x%02X messages that it logs as errors for some reason. Not sure we want that in the log. Also I get another warning with a static analyzer about the args... argument if I replace printf with a LOG call. IMF_LOG is sometimes called using a user-defined struct in the args... argument, and this brings up a "User-defined types should not be passed as variadic arguments" warning.

If someone can fix it to satisfy all the errors and warnings while keeping functionality that would be good, but it seemed to me that simply commenting it out would be enough for now.

By the way, something I noticed is there seems to be varied detection of the error in different environments. Compiling under GCC (in GitHub Actions) with -Werror=format-security, printf((format + "\n").c_str(), "%s", args...); does indeed seem to let it compile without error on the Linux build, but then the MinGW builds errored on message printing code in menu.cpp.

Meanwhile, the static analyzer I'm using (SonarLint for Visual Studio 2022) still detects the problem even when using printf((format + "\n").c_str(), "%s", args...);.

[Allofich]

We could exclude it from the build flags as you said. In general I'd prefer to satisfy the default warnings rather than disable them, or in this case, comment out the problem code.

[joncampbell123]

I'm going to push a commit to silence some compiler warnings.

At the same time, Silpheed doesn't seem happy with the IMFC emulation. No music is produced, it just gets really slow and laggy. Anything I should try?

[kcgen]

one thing to double check is that Sierra games have IMF.DRV and then setup to use it (after setup, the driver will be listed in resource.cfg).

Some games have very poor support (to the point of sounding broken).

Will try to find the vogons thread where these details are discussed.

[kcgen]

https://www.vogons.org/viewtopic.php?f=32&t=9555&start=220

And an XLS table attached in this thread: https://www.vogons.org/viewtopic.php?f=62&t=39709&start=20

Most of the 'native' flagged games should work (with the driver version + setup); the ones flagged 'modded' and patched are changed to make their messages compatible with the FB-01.

[joncampbell123]

one thing to double check is that Sierra games have IMF.DRV and then setup to use it (after setup, the driver will be listed in resource.cfg).

Some games have very poor support (to the point of sounding broken).

Will try to find the vogons thread where these details are discussed.

Yes, it does. Yes, I told the setup program to use it, and it wrote it to the config.

The game becomes sluggish because, according to the debugger, it's stuck polling one of the IMFC registers for a bit to flip (though with a countdown timeout).

[Allofich]

Same for me, it doesn't work in Silpheed. It doesn't work in DOSBox Staging, either.

[FredBezies]

Once again, gcc 13.1.1 is unhappy:

imfc.cpp:2017:10:   required from here
imfc.cpp:99:32: error: format not a string literal and no format arguments [-Werror=format-security]
   99 |         LOG(LOG_MISC,LOG_DEBUG)(format.c_str(), args...);
      |         ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~

[joncampbell123]

Once again, gcc 13.1.1 is unhappy:

imfc.cpp:2017:10:   required from here
imfc.cpp:99:32: error: format not a string literal and no format arguments [-Werror=format-security]
   99 |         LOG(LOG_MISC,LOG_DEBUG)(format.c_str(), args...);
      |         ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~

Why does GCC 13.x care so much whether it's a string literal? That completely ruins any custom printf-like functions where C++ variadic template Args are involved! This seems like security paranoia that goes a little overboard.

EDIT : Actually, is your system making the format-security warning into an error? What happens if the code is compiled with flags that make that a warning like -Wno-error=format-security ?

[maron2000]

Changing compiler option for this error was once discussed few years back #3039 Just to remind @FredBezies has forgot this.

[FredBezies]

Changing compiler option for this error was once discussed few years back #3039 Just to remind @FredBezies has forgot this.

I forgot everything about this bug.

[FredBezies]

Once again, gcc 13.1.1 is unhappy:

imfc.cpp:2017:10:   required from here
imfc.cpp:99:32: error: format not a string literal and no format arguments [-Werror=format-security]
   99 |         LOG(LOG_MISC,LOG_DEBUG)(format.c_str(), args...);
      |         ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~

Why does GCC 13.x care so much whether it's a string literal? That completely ruins any custom printf-like functions where C++ variadic template Args are involved! This seems like security paranoia that goes a little overboard.

EDIT : Actually, is your system making the format-security warning into an error? What happens if the code is compiled with flags that make that a warning like -Wno-error=format-security ?

Here are the CFLAGS and CXXFLAGS on my archlinux installation:

CFLAGS="-march=x86-64 -mtune=generic -O2 -pipe -fno-plt -fexceptions \
        -Wp,-D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security \
        -fstack-clash-protection -fcf-protection"
CXXFLAGS="$CFLAGS -Wp,-D_GLIBCXX_ASSERTIONS"

Did not try to build with -Wno-error=format-security. I will have to try this later today.

[grapeli]

@FredBezies These hardened flags, in my humble opinion, are suitable for system or server software exposed to all kinds of attacks. Sorry, dosbox-x is not one of those. They cause a decrease in efficiency by about ~5% (At least on my low-end hardware these differences are clearly measurable). Any warning flags are only good for developers of specific software, not for the end user. https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html I gave you a suggestion, either give up this one flag or take matters into your own hands and add a small trivial fix if the developers don't want to be so accommodating.

[FredBezies]

I tried with -Wno-error=format-security and it worked. This workaround is a good news. It only makes my PKGBUILD for dosbox-x-sdl2-git a little bigger.

[grapeli]

-mtune=generic? After all, you compile it only for your own needs and not for distribution to others, as for example often mediocre software contained in repositories of various software distributions.

[FredBezies]

-mtune=generic? After all, you compile it only for your own needs and not for distribution to others, as for example often mediocre software contained in repositories of various software distributions.

I propose git versions on AUR. So, it is not only for me :)

[grapeli]

@FredBezies You do not distribute ready-made binaries, you only give an average quality recipe on how to build software for your own needs.

[joncampbell123]

@FredBezies These hardened flags, in my humble opinion, are suitable for system or server software exposed to all kinds of attacks. Sorry, dosbox-x is not one of those. They cause a decrease in efficiency by about ~5% (At least on my low-end hardware these differences are clearly measurable). Any warning flags are only good for developers of specific software, not for the end user. https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html I gave you a suggestion, either give up this one flag or take matters into your own hands and add a small trivial fix if the developers don't want to be so accommodating.

That reminds me... I noticed GCC 9.3 likes to compile in the stack protector code (stackguard) by default. I wonder if DOSBox-X would run any faster if I disabled that too?

[grapeli]

Compiler flags hardening the binary very strongly (their cumulation) certainly affect the performance in benchmarks under DOSBox-X. Everything is, of course, a matter of needs, there are people who do not expect efficiency in the first place, because it may be secondary or less important, or it is sufficient anyway, etc. Although from my perspective, a loss of let's say 5% of performance is reason enough for me to give it up. Recovering it by modifying the source code is very difficult, by optimizing the compilation of the code it is much easier, which I recommend (PGO profiling and the use of LTO).

If someone is afraid to run DOSBox-X for security reasons, I recommend effective sandboxes (there are a lot of them to choose from under linux).

[kcgen]

Yes, it does. Yes, I told the setup program to use it, and it wrote it to the config.

👍

The game becomes sluggish because, according to the debugger, it's stuck polling one of the IMFC registers for a bit to flip (though with a countdown timeout).

@Allofich , were there testable points during integration when it worked OK in X?

Before adding it to Staging, I gave the IMFC cpp a try with DOSBox SVN and it worked ok (granted there was a compiler syntax issue involving a template class).

Unfortunately I don't have that SVN + IMFC tree, but can reproduce it to help. (I recall it similarly working in Staging without any functional or emulation changes).

[Allofich]

I only ever tested Silpheed with the current DOSBox-X and DOSBox Staging code, after seeing the report here that it didn't work. During integration I only tested one game, Codename: ICEMAN, and it worked fine (and still works fine). That was using your modified imfc.cpp (cleaned up but before being more heavily modified to fit in with DOSBox Staging). I never tested with the original unmodified code.

Incidentally, when I test Codename: ICEMAN in DOSBox Staging, it sounds a little different from DOSBox-X, somewhat higher-pitched. I don't know much about the sound code, but the only difference I really noticed while converting the code was with the low-pass filter. That part was slightly different so I had to convert it to DOSBox-X style. While doing so I saw that other code in DOSBox-X that calls the low-pass filter (for Soundblaster) also calls SetSlewFreq, but I didn't know if I should do that for the IMFC (and the result in testing sounded OK to me) and so I didn't touch it, but perhaps that should be called, or some other adjustment is needed, if the implementation in DOSBox-X is too low-pitched.

[joncampbell123]

SetSlewFreq and the lowpass code is not meant to affect how fast the mixer channel plays, they're just parameters that affect sample interpolation and filtering. You don't have to use those functions.

[Allofich]

Revisiting the Silpheed issue, the copy I tested with may have been bad. I noticed that even after selecting the IBM Music Feature Card in the installation program, when I checked RESOURCE.CFG it did not have IMF.DRV in it (even after manually adding it, the IBM Music Feature Card still did not work in the game, though). I then noticed that when you start the game and the big robot head is talking, the graphics around its moving mouth would corrupt, and no subtitles appeared (I didn't know before that there were supposed to be subtitles here so didn't notice their absence).

I got another copy that is supposed to be good and using this one, IMF.DRV gets set by the installation program and the graphics bug and missing subtitles are fixed. With this version, there is still no music in DOSBox-X with the IBM Music Feature Card selected, but sound effects do work. However, in DOSBox Staging, the music with this copy of the game does work.

[Allofich]

Actually with the good copy, IBM Music Feature Card music in Silpheed does in fact work in DOSBox-X. For the comment I made just above, I accidentally tested with a build of DOSBox-X that I think was from just before I added the IBM Music Feature Card support. Sorry for the confusion.

I'm guessing that @joncampbell123 may have tested with the same copy of Silpheed that I did previously.

[Allofich]

In fact the music does work even with the copy of Silpheed that I suspect was bad. You just need to open RESOURCE.CFG and replace IBMCARD.DRV, which is the driver for the "IBM Audio/Joystick Card" with "IBM three-voice music", with IMF.DRV, the driver for the "IBM PC Music Feature card".

[Allofich]

@joncampbell123 If you can confirm that editing the RESOURCE.CFG file as mentioned above allows music in Silpheed to work, I think we can close this (assuming the compiler error is a non-issue).