🔒 Locked

How does this ever work.

Started by ApochPiQ Dec 6, 2016 at 11:13 PM 13 replies 11.1k views

Original Post

Dec 6, 2016 at 11:13 PM

This is my favorite (read: least favorite) thing from the past couple of weeks.

It occurs only when running under Application Verifier, which is annoying, because it's masking another bug that I want to catch using AppVerifier, but I can't, because this crash starts happening first.

Here's the lovely code responsible (only slightly paraphrased):

ImportantHandle::ImportantHandle (void * rawData, unsigned sizeOfData) {
    if (sizeOfData) {
        this->ImportantPointer = rawData;
        // Plus some inconsequential stuff
    }
}

Spot the bug!

That's right: if you pass in a zero-size "blob", you get an undefined (aka. garbage) value in your ImportantPointer. What could go wrong!!!

The saddest part is, this is just one of many bugs in the program in question. Sometimes, it will just freeze in an infinite loop trying to reacquire a lost DirectX device, failing, and trying again without changing any state.

Other times, it'll crash in random areas with heap corruption - the bug I'm trying to chase - but not reliably, without AppVerifier. And of course AV causes it to barf in a dozen other creative ways instead.

It really is a minor miracle that I'm not yet bald.

Nypyren

Dec 6, 2016 at 11:43 PM

It really is a minor miracle that I'm not yet bald.

My theory is that our hair has evolved to use blinding rage as a growth medium.

ApochPiQ

Dec 7, 2016 at 12:55 AM

This code is a total gold mine.

Just found a place where we allocate N bytes, then placement-new an object of < N bytes into the buffer. No big deal, except we then access the buffer as if it were an object of... N bytes.

If you run with the debug allocator, the N bytes are zero-initialized. If you run under appverifier (or in Release, you know, just like you would for any software that's to be... released) the space between the small overlay object and the remainder of the N-byte buffer is garbage.

Servant of the Lord

Dec 7, 2016 at 1:30 AM

by 'if(sizeOfData)', did you mean 'if(rawData)'?

i.e. you were checking that the pointer wasn't null, but not that it was of non-zero size?

ApochPiQ

Dec 7, 2016 at 1:31 AM

No.

conquestor3

Dec 7, 2016 at 3:20 PM

FWIW I've worked 4 years in a very high stress position and went bald in my mid/late 20's. It's not so bad.

Sounds like you need to re-implement whatever was being tried there.

Zaoshi Kaba

Dec 7, 2016 at 8:10 PM

FWIW I've worked 4 years in a very high stress position and went bald in my mid/late 20's. It's not so bad.

That's why I'm not looking forward to any promotions. I just want to remain yet-another-programmer.

ApochPiQ

Dec 8, 2016 at 12:32 AM

Here's a juicy tidbit for posterity:

When running under Application Verifier, calls to HeapAlloc with the HEAP_ZERO_MEMORY flag will lie to you silently and return you NON-ZEROED pages.

This means that if you have any behavior relying on HEAP_ZERO_MEMORY, it will start blowing up in spectacular ways under AppVerifier. For extra fun, make sure the expected-to-be-zero data is used as a bitfield, preferably one indicating what array members are valid.

Days like this make me wish I had the discipline necessary to learn a completely different profession.

samoth

Dec 8, 2016 at 12:06 PM

Have you considered using Dr.Memory? This is basically a comprehensive (read as: no WTF) alternative to Valgrind with some extras which also works under Windows. I've found it to be immensely, stunningly helpful.

Although strange "dunno why" crashes where I cannot immediately pinpoint the reason are kinda rare for me (luckily, not having to deal with someone else's code of such a kind as the one you posted!), whenever they do happen, Dr.Memory's output looks something like:


15 possible errors (but probably false positive), see possible-blahblah.log
1 possible leak (but probably harmless), see possible-blahblah.log
2 leaks, blah blah, see blah. log
1 error in line source.cpp, line 12345 : blah blah
 
crash in blah.exe at 12345678 : blubb.cpp line 456789, accessing blah, 2 bytes
 
5 system handles not closed, blah blah, see blah.log

... which basically means 2 minutes spent fixing it, not 2 days. No PhD needed to decipher the output either.

Sik_the_hedgehog

Dec 9, 2016 at 8:09 PM

by 'if(sizeOfData)', did you mean 'if(rawData)'?

i.e. you were checking that the pointer wasn't null, but not that it was of non-zero size?

I think the check was that if the size is zero, then the data doesn't exist (hence no pointer is needed).

Servant of the Lord

Dec 10, 2016 at 6:52 AM

I think the check was that if the size is zero, then the data doesn't exist (hence no pointer is needed).

Ah, so 'this->ImportantPointer' is uninitialized, and the correct code should've been e.g: else { this->ImportantPointer = nullptr; }.

frankusthenerd

Aug 18, 2017 at 2:06 PM

Level design rules. A game is only as good as its levels and this is one category that I'm spending time to learn myself. The player is not going to care about the code. Does the character move? If yes, then you're good. The excitement of playing the level is a big factor. Are there lots of secret areas? Is the level challenging. I recently learned that by adding warp pipes to a level it is much more fun due to the access of secret areas. So I'd say learn level design more and coding second.