Reloading M4 with empty STANAG mag crashes game

[mightyagrippa]

Version is ge27b38d. Windows tiles build 4425. To reproduce, start as an evacuee military recruit, put last 3 points in strength, start game, open inventory, select M4, attempt to reload.

Multiple players in IRC reported same issue. Stack trace was requested in IRC. Not sure if this is that, but save me stackoverflow:

ntoskrnl.exe+0x75daa
ntoskrnl.exe+0x2e980
ntoskrnl.exe+0x657a1
ntoskrnl.exe+0x66c57
ntoskrnl.exe+0x78a0d
ntoskrnl.exe+0x77d1a
ntoskrnl.exe+0x369a0f
ntoskrnl.exe+0x396739
ntoskrnl.exe+0x72853
wow64cpu.dll!TurboDispatchJumpAddressEnd+0x6c0
wow64cpu.dll!TurboDispatchJumpAddressEnd+0xf5
wow64.dll!Wow64SystemServiceEx+0x1ce
wow64.dll!Wow64LdrpInitialize+0x42a
ntdll.dll!RtlUniform+0x6e6
ntdll.dll!EtwEventSetInformation+0x14e0c
ntdll.dll!LdrInitializeThunk+0xe
ntdll.dll!NtWaitForMultipleObjects+0x15
kernel32.dll!WaitForMultipleObjectsEx+0x8e
kernel32.dll!WaitForMultipleObjects+0x18
kernel32.dll!GetApplicationRecoveryCallback+0x2a7
kernel32.dll!GetApplicationRecoveryCallback+0x166
kernel32.dll!UnhandledExceptionFilter+0x161
kernel32.dll!UnhandledExceptionFilter+0xe0
ntdll.dll!RtlKnownExceptionFilter+0xb7
ntdll.dll!RtlInitializeExceptionChain+0x36

[ejseto]

On line 171 of item_location.cpp:

item obj = what->split( qty );

But on line 133:

what = nullptr;

[Malkeus]

I am unable to reproduce this, using same version/build in an unmodded world on a windows 7 64bit system. My guy just reloads his stuff and then ran outside and found a hulk, who violated him with a brick wall. Poor random guy.
Screenshot from before his untimely demise, load the m4 with empty mag, take out the mag, load the mag, load the m4 with loaded mag. And fire.
https://i.imgur.com/4zn5mjQ.png

P.S. I'm a derp. But hey, at least I covered all the cases, neh?

[Malkeus]

ejseto, doesn't line 172 check whether obj is null and do stuff if it is?

[ejseto]

That's too late, you've already dereferenced a null pointer by that point.

[Malkeus]

hmm..Maybe he meant to use 'it' there, since he sets it = what on 132. I get lost when people use incredibly general variable names and don't comment.

edit:
But if dereferencing a null pointer is a BAD THING then the entire if statement below 171 seems kinda hinky.
I had to google what dereferencing a null pointer was. It seems very bad indeed. Learning is !FUN!

[mightyagrippa]

@Malkeus that's odd, I'm getting this error on 64 bit win7 with an unmodded world. One difference I can see is that you seem to have your config set up while I'm getting the error on a completely default install.

[mugling]

@ejseto that looks a very likely candidate

[chaosvolt]

Tested:

• Attempting to reload a crossbow causes crash.
• Attempting to reload symbol of judgement (Arcana mod) causes crash.
• Attempting to reload single-barrel shotgun causes crash.
• Attempting to reload Ruger LCR .22 causes crash.
• Attempting to reload a PPSh-41 (first example of a gun that has yet to be magazine-ified I could find) causes crash.

Seems to affect more than just magazine-fed guns.

[mugling]

Seems to affect more than just magazine-fed guns.

Affects all reloading from inventory

[chaosvolt]

To clarify, I tested wielding a crossbow and reload that way, it crashed in that instance too. Not just from inventory.

Unless you're referring to whether the AMMO is in inventory versus on the ground, and I can't think of any reason why that'd be the cause. o.O

[chaosvolt]

Small problem.

Build 4435 should have the changes in that PR, right?

EDIT: Build 4436 DEFINITELY should by now, and yet it doesn't.

[chaosvolt]

I stand corrected. This does not seem to have been fixed.

[mugling]

You need to resolve your doubt about whether you are testing against #15465 before I can look into this any further

[Coolthulhu]

Can't replicate

[chaosvolt]

It's been an hour, I've tested the last several, most recent builds. That PR should already be in the build by now.

[Malkeus]

that commit landed in 4434
http://ci.narc.ro/view/Cataclysm-DDA/job/Cataclysm-Matrix/changes

[chaosvolt]

And this is still present in 4436. -_-

[mugling]

Can't replicate

Neither can I

Unless you're referring to whether the AMMO is in inventory versus on the ground

Yes, can you test this with ammo from the ground or a vehicle

[chaosvolt]

I just tested this with a crossbow, with ammo in inventory. In both tiles build and ASCII. Again, 4436.

And then I dropped the bolts on the ground and tried it. Same thing, both when right underfoot and in an adjacent space.

I'll double-check with AR-15s or other magazine-fed firearms. I'm now confused as to whether this issue needs to be re-opened, or if it's just #15462 instead.

[mugling]

@mightyagrippa unfortunately that stacktrace isn't going to be helpful. It would be useful if you could try the fix from #15465

@chaosvolt a segfault is always a valid bug. there is no need to post screenshots.

[chaosvolt]

Ah. Still, I'm wondering how this can't be replicated. In any case, it seems magazine-fed weapons are also subject to this bug.

When you attempted to replicate this, did you use the latest build from the site after this was merged, or something else?

[mugling]

I'm compiling from source and neither myself nor @Coolthulhu are building for windows.

What is the last build that doesn't have the fault for you?

[chaosvolt]

Hmm. Which suggests a Windows-specific issue then? And I'd need to start testing to see how far back this goes. @_@

[Coolthulhu]

Could be a compiler thing.

Does it happen before or after selecting ammo (when more than one option is available)?

[chaosvolt]

On the plus side, I've found that build 4419 seems to be the latest build without this issue. Not sure if this would prove useful.

EDIT: And finally tested with multiple sorts of ammo of the same type. Crashes on hitting reload, rather than prompting the ammo-select screen.

[chaosvolt]

Odd. I've been quick-testing this with pure defaults, yeah. Not sure what would be the cause. This bug is getting weirder and weirder.

[ejseto]

It's caused by the window size. Default 80x25 crashes. 180x65 doesn't.

[Malkeus]

This sounds familiar. Wasn't there a similar crash a few months back?
On Feb 18, 2016 10:12 PM, "ejseto" notifications@github.com wrote:

It's caused by the window size. Default 80x25 crashes. 180x65 doesn't.

—
Reply to this email directly or view it on GitHub
#15450 (comment)
.

[chaosvolt]

Oh for the love of...yeah, there was an issue like that. Trying to recall which it was. And yep, setting it to my favored 100x40 causes no problem with crossbows. Gonna check magazines though.

[Malkeus]

Ah,I remember now.that bug crashed on startup with default screen settings
.never mind, false alarm.
On Feb 18, 2016 10:14 PM, "MalkeX" mst3ktoo@gmail.com wrote:

This sounds familiar. Wasn't there a similar crash a few months back?
On Feb 18, 2016 10:12 PM, "ejseto" notifications@github.com wrote:

It's caused by the window size. Default 80x25 crashes. 180x65 doesn't.

—
Reply to this email directly or view it on GitHub
#15450 (comment)
.

[chaosvolt]

Still related, in that both issues had the same absurdly arcane cause.

EDIT: Magazines and magazine-fed guns likewise cause no problems if window size is bumped up. Have I mentioned how WEIRD this issue is? XP

[ejseto]

If the window isn't at least 94 wide, it's not big enough for the pick ammo dialog. Presumably there's some check where the ammo dialog window doesn't get created if the game window isn't wide enough, and instead the game just passes around a null pointer assuming it's ammo dialog window pointer until it finally gets dereferenced.

On line 488 of ui.cpp:

window = newwin(w_height, w_width, w_y, w_x);

Using the default window size of 80, w_x == -7. Since w_x is the x coordinate of the window, newwin() returns NULL. On line 56 of cursesport.cpp:

    if (begin_y < 0 || begin_x < 0) {
        return NULL; //it's the caller's problem now (since they have logging functions declared)
    }

Gotta love that comment.

[chaosvolt]

Ah, that explains it.

[mightyagrippa]

@ejseto What an excellent comment. Sorry I wasn't here during the discovery, will check in tomorrow to see if more testing is needed.

[mugling]

Maybe Jenkins should also do debug builds with symbols on?

I haven't looked into this but if indeed no debug symbols are being provided you should consider reporting this as a separate issue.

On line 56 of cursesport.cpp:

Well identified @ejseto.

I'm going to patch the reloading dialog but would be surprised if this bug can't be reproduced in other dialogs at arbitrary screen sizes.

[mugling]

Can reproduce at 80x25

[ejseto]

The code that calculates the window size and position is in uimenu::setup() so I'd expect it to happen anywhere you try to make a window larger than the game window.

[Moonsabrt]

Yeah this issue is happening with me to i posted a forum bug at the garage in the cataclysm forums..But its not with an m4 its with any gun. As choasvolt said any reloading causes an error.
If this is a problem with the window size how would one change it?

[mugling]

The code that calculates the window size and position is in uimenu::setup() so I'd expect it to happen anywhere you try to make a window larger than the game window.

Yes, I'm just checking now but I think the reload code actually constraints w_x and w_y > 0 so the problem is going to need fixing further up

[Moonsabrt]

Oh gawd what is happening, welp the bug crashed my save file and world, and yes same thing any world and any game whether a folder freshly downloaded or one that i have been using for a while.

[mugling]

Yes, I'm just checking now but I think the reload code actually constraints w_x and w_y > 0 so the problem is going to need fixing further up

Actually it doesn't, see #15477

[mugling]

if (begin_y < 0 || begin_x < 0) { return NULL; // ...

Is there any reason this code couldn't constrain x and y >= 0 and issue a debugmsg rather than returning nullptr?

[ejseto]

Dunno, but I'd guess it's because it's not whoever-wrote-that-code's problem. There doesn't really seem to be an easy way to fix it though. You could say, "Well, why doesn't it create the window anyway, and just push the text off screen?" But who knows what that text is or how important it is, and you could just turn around and say, "Well don't try to create a window bigger than the game window, duh." You could also say, "Well what about word-wrap?" But that could just shift the problem from x to y. It'd also ruin your formatting.

I'd guess the official response would probably be to not make such (big text requiring such) big windows since they want the game playable in 80x25.

[Zireael07]

Could we have a note on 80x25 requirement in the contributing docs?
EDIT: And some sort of catch-all code that would prevent trying to create windows bigger than game windows, regardless of why it happens?

Also I think I remember Rivet saying that most players play on larger sizes and that we're slowly moving away from having to support 80x25...

[mugling]

we're slowly moving away from having to support 80x25...

It's fairly challenging to play at 80x25 in any case. That said the game shouldn't crash at any screen size.

[narc0tiq]

Maybe Jenkins should also do debug builds with symbols on?

I haven't looked into this but if indeed no debug symbols are being provided you should consider reporting this as a separate issue.

Or I could save you some time and specify this is working as intended. Making Jenkins builds with debug symbols would be both slower and result in much larger binaries.

[Moonsabrt]

...I am so confused by reading this.

[chaosvolt]

Actually it doesn't, see #15477

Behold the bug that will not die. ;w;

EDIT: Oh wait no, that was replying to a statement suggested that a small tweak wouldn't fix this, which means this will be fixed by that PR. I hope?

[Malkeus]

Can't get much smaller than changing a single character. C++ is !FUN!

[chaosvolt]

And by that you mean !!FUN!! of course.

[Moonsabrt]

........IS THERE A CONCLUSION TO THIS...SERIOUSLY ANYONE!!?!?!?

[Malkeus]

A fix was PRd in #15477. Not sure if it's been merged yet, but it will be
soon enough I imagine.
On Feb 19, 2016 7:07 PM, "Moonsaber" notifications@github.com wrote:

........IS THERE A CONCLUSION TO THIS...SERIOUSLY ANYONE!!?!?!?

—
Reply to this email directly or view it on GitHub
#15450 (comment)
.

[chaosvolt]

Yeah, that PR is still open. Until then? Beef up your window width.