SDL_mixer: fix mikmod malloc/free mismatch - cause of crashe

you can check on mercurial and see what patches have been added http://hg.libsdl.org/SDL_mixer/ seems the patch was added yesterday

R Manard wrote:> You?say I made some good points. Thank you. I remember times when fixes to sdl were offered?where it was a disaster to be avoided. Learning from the past I wanted to make the points that should have seemed obviouse, in my opinion. Blanks to fill in. Procedure to follow.?When I said that maybe the fix was the best thing anyone could do, it was?saying that there might?be no cause for any alarm and simply be top notch world class work that is now finished. Sam must think this is true. Having the familliarity?with sdl?I believe he opened it up and?understood it to be a true and current fix. That’s good enough for me.

I don’t like to talk about language or nationality. Sometimes I don’t use the word that I think it is. I don’t extend different treatment to people but assume everyone uses the wrong word sometimes.
?
If I download the mixer will the fix be in it? Does anyone know?

On Sun, Jul 22, 2012 at 8:24 PM, Jared Maddox <absinthdraco at gmail.com (absinthdraco at gmail.com)> wrote:

Date: Sun, 22 Jul 2012 17:14:21 -0500

From: R Manard <dranikist at gmail.com (dranikist at gmail.com)>
To: SDL Development List <sdl at lists.libsdl.org (sdl at lists.libsdl.org)>
Subject: Re: [SDL] SDL_mixer: fix mikmod malloc/free mismatch - cause
? ? ? of crashes on systems that check for that.
Message-ID:
? ? ? <CAKCUQMYdzdtYZRCkkLpivkv4gbZ=WsawwF_4wqj_XgWL87WshA at mail.gmail.com (WsawwF_4wqj_XgWL87WshA at mail.gmail.com)>
Content-Type: text/plain; charset=“iso-8859-1”

Sam applying the patch pretty much answers your points, but I figured
that I should point out some relevant details (especially since I
half-recall reading that you were French, and thus might need some
language tips).

On Sun, Jul 22, 2012 at 8:14 AM, Paul P Komkoff Jr <i at stingr.net (i at stingr.net)> wrote:

With the little help I tracked the crash down and wrote a test case
that reproduces it perfectly:

#include <mikmod.h>
main()
{
? ? ? ? MikMod_RegisterAllDrivers();
? ? ? ? MikMod_Init("");
? ? ? ? char* d = MikMod_InfoDriver();
? ? ? ? if (d) {
? ? ? ? ? printf("%s\n", d);
? ? ? ? ? free(d);
? ? ? ? }
? ? ? ? MikMod_Exit();
}

The crash happens at free(), where glibc will detect “double-free or
corruption”. This is happening because mikmod uses MikMod_malloc() to
allocate memory for return value, and MikMod_malloc() does some funny
things with allocations and alignments. Replacing free() with
MikMod_free() is enough to stop crashes.

If something sdl uses is broken and then something else uses sdl that would
change things or if your stuff now works but now most other people’s stuff
is broke because of the way it was fixed that would be bad.

You see that bit where mikmod has it’s own free() function, but SDL
wasn’t using it in the necessary situations? That needed to be fixed.
If programs or libraries were depending on that, (which is highly
unlikely, since it was producing crashes), then they need to be fixed.
The change to SDL (regardless of whether it works perfectly with the
first patch) is the correct thing to do, because SDL had the wrong
behavior. If the change breaks something, then it was the fault of the
thing that broke.

Preserving bugs because something uses them is WRONG. I realize that
Microsoft does it with Windows, but (especially with the early
versions, where such anecdotes seem the most common) Windows was
historically considered buggy and prone to viruses, partially because
of those intentionally preserved bugs. Better to deal with it up-front
(especially since API-breakage should still be expected in the 2.0
series) than to let it fester.

What if it’s an
old version and it was fixed two versions ago?

Then the patch gets ignored, unless it is an improvement on the old
fix, in which case Sam & co. decide whether to replace the old patch
with the new. But yes, Paul should have listed the actual version that
he applied the patch to.

One last thing. Your response was toned too harshly. In particular, it
was written as if you were trying to discourage Paul from posting
patches, and if I ignored that implication then it seemed like you
were panicking about the possibilities that you mentioned. I doubt
that you intended the first of those interpretations, but in case the
second was true, I’ll say this:

Stop worrying so much, the cost of those mistakes is very small so
there’s no problem if they happen. Even more importantly, nothing will
improve if nothing changes, and successfully crowd-sourced projects
move faster than ones with only two or three people working on them.

In the words of Douglas Adams: Don’t Panic.


SDL mailing list
SDL at lists.libsdl.org (SDL at lists.libsdl.org)
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org (http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org)

I only use release versions but thank you the sameOn Mon, Jul 23, 2012 at 5:46 AM, stevo5800 wrote:

**
you can check on mercurial and see what patches have been added
http://hg.libsdl.org/SDL_mixer/ seems the patch was added yesterday

R Manard wrote:

You say I made some good points. Thank you. I remember times when fixes to
sdl were offered where it was a disaster to be avoided. Learning from the
past I wanted to make the points that should have seemed obviouse, in my
opinion. Blanks to fill in. Procedure to follow. When I said that maybe the
fix was the best thing anyone could do, it was saying that there might be
no cause for any alarm and simply be top notch world class work that is now
finished. Sam must think this is true. Having the familliarity with sdl I
believe he opened it up and understood it to be a true and current fix.
That’s good enough for me.
I don’t like to talk about language or nationality. Sometimes I don’t use
the word that I think it is. I don’t extend different treatment to people
but assume everyone uses the wrong word sometimes.

If I download the mixer will the fix be in it? Does anyone know?

On Sun, Jul 22, 2012 at 8:24 PM, Jared Maddox <> wrote:

Quote:

Date: Sun, 22 Jul 2012 17:14:21 -0500

Quote:

From: R Manard <>
To: SDL Development List <>
Subject: Re: [SDL] SDL_mixer: fix mikmod malloc/free mismatch - cause
of crashes on systems that check for that.
Message-ID:
**
Content-Type: text/plain; charset=“iso-8859-1”

Sam applying the patch pretty much answers your points, but I figured
that I should point out some relevant details (especially since I
half-recall reading that you were French, and thus might need some
language tips).

Quote:

On Sun, Jul 22, 2012 at 8:14 AM, Paul P Komkoff Jr <> wrote:

Quote:

Quote:

With the little help I tracked the crash down and wrote a test case
that reproduces it perfectly:

#include **
main()
{
MikMod_RegisterAllDrivers();
MikMod_Init("");
char* d = MikMod_InfoDriver();
if (d) {
printf("%s\n", d);
free(d);
}
MikMod_Exit();
}

The crash happens at free(), where glibc will detect “double-free or
corruption”. This is happening because mikmod uses MikMod_malloc() to
allocate memory for return value, and MikMod_malloc() does some funny
things with allocations and alignments. Replacing free() with
MikMod_free() is enough to stop crashes.

Quote:

If something sdl uses is broken and then something else uses sdl that would
change things or if your stuff now works but now most other people’s stuff
is broke because of the way it was fixed that would be bad.

You see that bit where mikmod has it’s own free() function, but SDL
wasn’t using it in the necessary situations? That needed to be fixed.
If programs or libraries were depending on that, (which is highly
unlikely, since it was producing crashes), then they need to be fixed.
The change to SDL (regardless of whether it works perfectly with the
first patch) is the correct thing to do, because SDL had the wrong
behavior. If the change breaks something, then it was the fault of the
thing that broke.

Preserving bugs because something uses them is WRONG. I realize that
Microsoft does it with Windows, but (especially with the early
versions, where such anecdotes seem the most common) Windows was
historically considered buggy and prone to viruses, partially because
of those intentionally preserved bugs. Better to deal with it up-front
(especially since API-breakage should still be expected in the 2.0
series) than to let it fester.

Quote:

What if it’s an
old version and it was fixed two versions ago?

Then the patch gets ignored, unless it is an improvement on the old
fix, in which case Sam & co. decide whether to replace the old patch
with the new. But yes, Paul should have listed the actual version that
he applied the patch to.

One last thing. Your response was toned too harshly. In particular, it
was written as if you were trying to discourage Paul from posting
patches, and if I ignored that implication then it seemed like you
were panicking about the possibilities that you mentioned. I doubt
that you intended the first of those interpretations, but in case the
second was true, I’ll say this:

Stop worrying so much, the cost of those mistakes is very small so
there’s no problem if they happen. Even more importantly, nothing will
improve if nothing changes, and successfully crowd-sourced projects
move faster than ones with only two or three people working on them.

In the words of Douglas Adams: Don’t Panic.


SDL mailing list

http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org


SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org