Inconsistent behavior of lfs wrapper and implementation
Brought to you by:
sobukus
Menu
▾
▴
#328 Inconsistent behavior of lfs wrapper and implementation
The wrapper function for mpg123_feedseek() in lfs_wrap.c doesn't check the input_offset parameter for NULL and simply attempts to write to it. This behaivor differs from the actual implementation of mpg123_feedseek(), which does a null pointer check on the input_offset parameter.
This inconsistency has caused some breakage in Wine recently, where applications that do MP3 decoding crash in the wrapper function (see https://bugs.winehq.org/show_bug.cgi?id=52191).
Damn. Wine is a case where the LFS wrappery rears its head again. Oh how I regret having introduced
off_tin public API. One of the most expensive mistakes of my life.Although … even if emulating 32 bit Windows API, I wonder if wine really should call libmpg123 with 32 bit file offset API. Just for not having to do the overflow checking itself?
Anyhow, you're right in that the check in the wrapper was missing for a NULL pointer being handed in. I fixed that an some other place in the same file in the current trunk (rev. 5007). Thanks for the pointer (pun not intended, but acknowledged just now). I must admit that I don't regularily test this wretched code … if I'd write this anew, I'd just enforce int64_t in the API. I like the idea of having a system-specific off_t of some width, but the decision to have this type mapped to differing widths on the same system was BAD BAD BAD BAD BAD BAD BAD REALLY BAD!
Still, probably Wine should ensure to hand in a valid address for the input offset value. Or is it DX8 itself triggering that in a way that one cannot avoid it? Calling the feedseek without reading the input offset return value is a rather strange affair, anyway. The whole point of the routine is to a) prepare the decoder for the new position and b) give the caller an offset in the input stream to jump to. When you don't want the input offset stored, why bother calling?
If you can confirm that the current svn trunk or https://mpg123.org/snapshot works (moves the crash to a different place with the game;-)?
Thank you for your quick reply and fix. I built Wine with the change and the game appears to be fixed now.
I agree with your stance on the wrapper, and it seems Wine sees it that way too. The wrapper was disabled and completely removed from Wine's in-tree version of libmpg123 in a commit earlier. It was probably not supposed to be enabled in the first place (I assume it happened when they moved the library in-tree).
I just had a look at the internal copy of libmpg123 in wine … they chose the generic decoder with dithering. This is the slowest possible choice. Possibly the best-sounding, OK, but the ditching of any optimizations like at least SSE hurts. I created libmpg123 to get rid of the need for various projects to include mpg123 code as various outdated copies of the so-called mpglib. Somehow sad that we're back there, again.
If you have to vendor the code and use some specific configuration, it at least would be nice if the vendoring is automated enough so that things don't start to get stale in case any decoder issues are fixed upstream (yes, there are fuzzers finding things from time to time). I'm also not sure how well the fixed config.h works on various platforms.
In the end, the wine project should know what it does and why, but for me it's a bit backwards to have code duplicated without history/feedback. You can get your choice of build with options to the official configure script (including disabling of LFS wrappers). Of course, on my Linux systems, I'd expect wine to use the system's libmpg123. The API you use is stable enough. You can define
_FILE_OFFSET_BITS=32to get the wrappers using long arguments — and for the bug at hand just ensure that you don't hand in NULL where there is a mandatory output argument.Anyhow, would be nice to see the rationale for forking libmpg123. The initial commit from October doesn't say why.