Audio::initInput & Audio::initOutput Use After Free

[kdhp]

Brief Description

OS: FreeBSD 12-CURRENT
qTox version: 1.5.2-143-g60af778 & 1.5.1
Commit hash: 60af778
toxcore: 0.151112
Qt: 5.6.1
Hardware: amd64
Compiler: clang 3.8

Reproducible: Always

Steps to reproduce

1. Run qTox with junk set to free or true: MALLOC_CONF='junk:free' qtox
2. Go to Settings-Audio/Video (to cause a call to Audio::initOutput).

Observed Behavior

The audio device fails to open:

[06:00:00.000 UTC] src/audio/audio.cpp:345 : Debug: Opening audio input "OSS Default"
[06:00:00.000 UTC] src/audio/audio.cpp:363 : Warning: Failed to initialize audio input device: "OSS Default"
[06:00:00.000 UTC] src/audio/audio.cpp:293 : Warning: Failed to subscribe to audio input device.

Because junk is being passed to alcOpenDevice:

# dtrace -p <qtox-pid> -n 'pid$target::alcOpenDevice:entry/arg0!=0/{printf("%s",copyinstr(arg0))}'                                                                                                          
dtrace: description 'pid$target::alcOpenDevice:entry' matched 1 probe
CPU     ID                    FUNCTION:NAME
  0  75522              alcOpenDevice:entry ZZZZZZZZZZZZ...

Note: junk:free fills freed memory with 0x5A octets (ASCII Z)

Expected Behavior

The open succeeds:

[06:00:00.000 UTC] src/audio/audio.cpp:298 : Debug: Subscribed to audio input device [ 1 subscriptions ]

With the correct argument:

# dtrace -p <qtox-pid> -n 'pid$target::alcOpenDevice:entry/arg0!=0/{printf("%s",copyinstr(arg0))}'                                                                                                          
dtrace: description 'pid$target::alcOpenDevice:entry' matched 1 probe
CPU     ID                    FUNCTION:NAME
  0  75522              alcOpenDevice:entry OSS Default

Cause

In both Audio::initInput and Audio::initOutput a QByteArray is created, without being assigned to a variable, and tmpDevName is set to a pointer internal to the array. After this the array is destroyed, the string freed, and the now invalid pointer is used as the argument for alcOpenDevice.

Additional Info

malloc.conf(5)

[antis81]

Duh!! Nice one! Thanks for spotting this!

One possible solution:

// replace the "ALchar* tmpDevName =…" line. 
//    const ALchar* tmpDevName = deviceName.isEmpty()
//                               ? nullptr
//                               : deviceName.toUtf8().constData();
QByteArray tmpDevName = deviceName.toUtf8();
alInDev = alcCaptureOpenDevice(tmpDevName.isEmpty() ? nullptr : tmpDevName.constData(),
                               sampleRate, stereoFlag, bufSize);
…

@kdhp Would you provide the fix please? I don't have the time currently. Note, that this will fix #3721 and probably #3538 and #3603 as well.

EDIT: ~~QByteArray is always \0 terminated. Corrected that.~~Actually this is only true for QByteArray::data(). It is ok anyway.`

[kdhp]

@antis81 I doubt that this issue would have any consistency without junking, so it may be unrelated to #3721. I also checked the other two and can confirm that they are unrelated to this.