Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when using the VoiceRecorder with voice target shortcuts #2864

Merged
merged 3 commits into from Feb 26, 2017

Conversation

@mkrautz
Copy link
Member

commented Feb 22, 2017

This patch series fixes #2863 (Mumble segfaults during recording.)

During a VoiceRecorder session, when pressing a voice target shortcut, it was possible to set iPrevTarget to -1. That, combined with the fact that flushCheck() in AudioInput didn't properly check whether the values for iTarget and iPrevTarget were valid. This caused a voice packet with the an invalid flag byte to generated. This caused a crash in AudioOutputSpeech's needSamples(), because it would treat the bad packet as a Speex packet. Basically -- the logic was that anything that isn't CELT or Opus must be Speex -- which obviously isn't true: invalid packets would be treated as Speex.

The first commit in this series fixes the problem with missing validation for iTarget/iPrevTarget.

The second commit fixes the crash-site: When a bad packet was generated by the VoiceRecorder due to iPrevTarget being -1, AudioOutputSpeech::needSamples() would treat the bad packet as Speex, and would try to decode it, and fail. We now explicitly check whether the message type is Speex, before treating a packet as Speex.

The final commit adds an additional guard to AudioOutputSpeech, to catch invalid voice packets before they get into the audio subsystem.

@mkrautz mkrautz force-pushed the mkrautz:flushcheck-itarget branch 4 times, most recently from e5fb3cb to 75d8eef Feb 22, 2017

@mkrautz mkrautz changed the title WIP: fix @tatokis's VoiceRecorder crash Fix crash when using the VoiceRecorder with voice target shortcuts Feb 22, 2017

@mkrautz mkrautz requested review from hacst, Kissaki and davidebeatrici Feb 22, 2017

/// UDPMessageTypeIsValidVoicePacket checks whether the given
/// UDPMessageType is a valid voice packet.
inline bool UDPMessageTypeIsValidVoicePacket(MessageHandler::UDPMessageType umt) {
switch (umt) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 22, 2017

Author Member

23:28:30 mkrautz: ../../src/Message.h:60:9: warning: enumeration value ‘UDPPing’ not handled in switch [-Wswitch]

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 22, 2017

Author Member

We should probably add MessageHandler::UDPPing and return false for it.

If we add default, we won't get warnings for missing entries in the future...

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 22, 2017

Author Member

Fixed.

@mkrautz mkrautz force-pushed the mkrautz:flushcheck-itarget branch 2 times, most recently from efd62af to 93c19ec Feb 22, 2017

@Kissaki
Copy link
Member

left a comment

I’m not sure I’m that qualified, but anyway.

Commit 1 looks plausible

Commit 2 makes sense. Is silently ignoring the else case what we want though? There's some followup code as well, and we won't notice unexpected message types.

Commit 3 has a typo "we check they they're". Also I would simplify "However, some packets, we generate ourselves" to "However, we generate some packets ourselves".

Commit 3 seems plausible.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2017

Commit 2 makes sense. Is silently ignoring the else case what we want though? There's some followup code as well, and we won't notice unexpected message types.

Since packets from the server will never reach this place, I think you're right.
It doesn't hurt to put a qWarning there.

Commit 3 has a typo "we check they they're". Also I would simplify "However, some packets, we generate ourselves" to "However, we generate some packets ourselves".

Will fix.

mkrautz added 3 commits Feb 22, 2017
AudioInput: check iTarget and iPrevTarget for errors before use in fl…
…ushCheck().

@tatokis reported a voice recorder crash on IRC.
(Also filed as #2863)

The main culprit was missing error checking in flushCheck() for the values
of iTarget and iPrevTarget.

The problem is that iPrevTarget can be set to -1 in error situations,
when using voice target shortcuts. (Whispers and shouts.)

When that happens, the message type output by flushCheck() will be set
to an invalid value (7), because iPrevTarget is -1.

In flushCheck(), when the packet is a terminator, we set the packet's flags
to the value of iPrevTarget. However, in some error states, iPrevTarget is -1.
When flags is set to -1, all bits are high (say, 0xffffffff on 32-bit
systems). Since all bits are high, our attempt to set the message type in
the flags byte fail, because we attempt to splice it in there via binary
OR.

The result is a packet with an invalid message type of 7 gets into our
audio subsystem and wreaks havoc. Due to mistakes in other code in
AudioOutputSpeech, that invalid value could cause a crash. (The problem
was that we expected that all packets that weren't Opus or CELT to be
Speex. Even 'unknown' message types. This will be fixed in a separate
commit.)

Fixes #2863
AudioOutputSpeech: only process Speex packets as Speex.
Otherwise, a malformed packet  could lead to a crash.

Note that this only affects packets generated by Mumble itself,
such as local loopback packets and voice recorder packets.

Packets from the network are filtered in such a way that a
packet with an invalid type will never be able to enter the
audio subsystem -- it is discarded in the network subsystem.
Audio, AudioOutput: guard against invalid packet types in AudioOutput…
…::addFrameToBuffer().

For packets we receive from Murmur, we check that they're a valid voice
packet on receipt.

However, some packets are not received by Murmur, but generated by the
client itself: packets for RecordUser, and local loopback packets.

If we mess up their type due to a bug, we'd previously allow them into the
AudioOutput system. This commit discards them before they get there.

@mkrautz mkrautz force-pushed the mkrautz:flushcheck-itarget branch from 93c19ec to f4dfc4a Feb 25, 2017

@mkrautz mkrautz merged commit 329dd4e into mumble-voip:master Feb 26, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.