OS X: crash on startup, at "Loading BDF font", r6658

[ArthurDenture]

Simutrans hangs for me on startup at the current head revision. With -debug 5, the last lines printed to the console are:

Loading BDF font font/Prop-Latin1.bdf with 256 characters
2013-08-25 12:57:45.089 sim-pristine-head[54358:707] *** -[__NSArrayM objectAtIndex:]: index 18446744073709551615 beyond bounds [0 .. 41]

It appears that this message occurs when returning from display_load_font() in simgraph16.cc. So it's symptomatic of a buffer overrun or other memory problem in load_font, such that the stack gets corrupted.

Update: I've bisected this down to revision 6592, "Max-Max: next part of themes support". Unfortunately, that's a ginormous change, in part by mixing together some significant refactorings with a ton of unrelated code style changes, so I don't have much clue where the bug might be within it.

[kierongreen]

Thanks - will try to look at this more over next couple of days if someone else doesn't find a fix first

[ArthurDenture]

I've finally hunted down what exactly causes the crash. It's in music/core-audio_midi.mm: inside dr_midi_pos(), ultimately triggered by check_midi() in simmain.cc, nowPlaying is -1 and is used as an array index. The clue here is that the crash is in objective-C land, which really only is used for sound and midi on mac, and the crash went away if I disabled midi by replacing core-audio_midi.mm with no_midi.cc. I don't know why gdb showed the traceback as being in font code, except that the actual crash occurred not long after that function)

The regression in https://github.com/aburch/simutrans/commit/3a22d4cb5d39c4acb59856ef2a8a24c5abdda865 is that the old version of check_midi() first checked midi_get_mute() and did nothing if midi was muted. The new version doesn't have that check until later, and before that it calls dr_midi_pos(), causing the crash.

The fix is straightforward: dr_midi_pos() should be guarded with a check for now_playing != -1.

I do have to say that it would be beneficial to avoid large commits that touch many unrelated areas. This regression was really difficult to find; it would have been much easier if the change to the audio subsystem were in a commit by itself, with a commit message explaining what was changed and why.

[kierongreen]

Thanks for that work debugging. I'm not sure that is actually the fix to use though. It does fix the crash - however Max specifically changed the code here saying so in a comment and I'm not sure exactly why.

[ArthurDenture]

Sorry, I was unclear: the guard goes inside dr_midi_pos. I've attached a diff.

It looks like the other midi backends are already safe against calling dr_midi_pos while music is not playing. So this just makes the Core Audio one compliant in the same way.

[prissi]

Incorporated, thank you