AES_encrypt crash

[rmprosser]

Our app has crashed a number of times with this stack trace:

AES_encrypt + 452
MumbleClient::CryptState::ocb_encrypt(unsigned char const*, unsigned char*, unsigned int, unsigned char const*, unsigned char*) + 66
MumbleClient::CryptState::encrypt(unsigned char const*, unsigned char*, unsigned int) + 72
-[MKCryptState encryptData:] + 166 
-[MKConnection _sendUDPMessage:] + 90

I believe it is triggered by a poor internet connection but haven't been able to reproduce. Any insights appreciated.

[wtmoose]

@0xABC Little late to the party, but did you (or anyone else) ever find a resolution to this crash?

[rmprosser]

@wtmoose unfortunately nope

[wtmoose]

I'm not a C++ guy, but I believe the issue is that the CryptState variable is never initialized:

struct MKCryptStatePrivate {
	CryptState cs;
};

It seems they're relying on the no argument initializer being invoked implicitly. But it isn't ever called. I added this line in [MKCryptState init] and I haven't seen this crash since:

_priv->cs = CryptState();

Would be interested to hear from a C++ person if this change isn't necessary.

[rmprosser]

That shouldn't be necessary because cs is not a pointer. That declaration should call the default constructor implicitly. Of course this is Objective-C not pure C++ so who knows.

[mkrautz]

No, I believe @wtmoose's analysis is correct. The struct is malloc'd, and I never initialize CryptState properly. Oops.

[wtmoose]

I did verify in the debugger that it isn't initislized without my change.

[mkrautz]

ISTM that the easiest/cleanest fix is to replace the MKCryptStatePriv struct in MKCryptState with a pointer to CryptState instead. Then new & delete it in init/dealloc.

PRs welcome. Otherwise I'll fix it soon.

[rmprosser]

Cool! I guess cs doesn't have a user-declared default constructor. I wonder why this is only a problem some of the time though. Anyway thanks @wtmoose and @mkrautz

[mkrautz]

@wtmoose Even if CryptState is not properly initialized, it should not trigger a crash. The memory is there, and the underlying data will be initialized when connecting to a server: when the server sends the CryptState message, everything (except decrypt_state) will be initialized.

So while I agree that CryptState not being properly initialized is a bug, I am not convinced that it really fixes the issue. I know this is a hard-to-reproduce problem (at least, it has been for me), but are you sure it really fixes the crash?

[mkrautz]

Anyway, feel free to test PR #54 to see if it helps! Thanks.

[wtmoose]

No, I'm not sure. I made two changes at the same time and haven't had time to test them independently.

The second change is related to #55. We were initially calling _pingTimerFired from the main thread and the second change was to move that call to our MKConnection thread. I'm almost certain this was causing a crash. I left the CryptState change in because of the amount of time it would take to retest.