Skip to content

Commit 3186e53

Browse files
devopvoiddevopvoid
committed
fix: check pinned arrays and frame lengths in the audio and video buffer natives
Null results of GetByteArrayElements went unchecked, AudioProcessing leaked both arrays on its early return, and processStream read and wrote past arrays shorter than the configured frame; it now returns kBadDataLengthError for them.
1 parent 07b1fad commit 3186e53

5 files changed

Lines changed: 132 additions & 130 deletions

File tree

‎webrtc-jni/src/main/cpp/src/JNI_AudioConverter.cpp‎

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,28 +19,33 @@
1919
#include "media/audio/AudioConverter.h"
2020
#include "JavaUtils.h"
2121

22+
#include <memory>
23+
2224
JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_audio_AudioConverter_convertInternal
2325
(JNIEnv * env, jobject caller, jbyteArray src, jint nSrcSamples, jbyteArray dst, jint nDstSamples)
2426
{
2527
try {
2628
jni::AudioConverter * converter = GetHandle<jni::AudioConverter>(env, caller);
2729
CHECK_HANDLE(converter);
2830

29-
jboolean isDstCopy = JNI_FALSE;
30-
31-
jbyte * srcPtr = env->GetByteArrayElements(src, nullptr);
32-
jbyte * dstPtr = env->GetByteArrayElements(dst, &isDstCopy);
33-
34-
converter->convert(reinterpret_cast<const int16_t *>(srcPtr), nSrcSamples, reinterpret_cast<int16_t *>(dstPtr), nDstSamples);
35-
36-
if (isDstCopy == JNI_TRUE) {
37-
jsize dstLength = env->GetArrayLength(dst);
31+
auto releaseSrc = [env, src](jbyte * bytes) {
32+
env->ReleaseByteArrayElements(src, bytes, JNI_ABORT);
33+
};
34+
std::unique_ptr<jbyte, decltype(releaseSrc)> srcPtr(env->GetByteArrayElements(src, nullptr), releaseSrc);
35+
if (!srcPtr) {
36+
return;
37+
}
3838

39-
env->SetByteArrayRegion(dst, 0, dstLength, dstPtr);
39+
// Mode 0 writes the samples back when the VM handed out a copy.
40+
auto releaseDst = [env, dst](jbyte * bytes) {
41+
env->ReleaseByteArrayElements(dst, bytes, 0);
42+
};
43+
std::unique_ptr<jbyte, decltype(releaseDst)> dstPtr(env->GetByteArrayElements(dst, nullptr), releaseDst);
44+
if (!dstPtr) {
45+
return;
4046
}
4147

42-
env->ReleaseByteArrayElements(src, srcPtr, JNI_ABORT);
43-
env->ReleaseByteArrayElements(dst, dstPtr, JNI_ABORT);
48+
converter->convert(reinterpret_cast<const int16_t *>(srcPtr.get()), nSrcSamples, reinterpret_cast<int16_t *>(dstPtr.get()), nDstSamples);
4449
}
4550
catch (...) {
4651
ThrowCxxJavaException(env);

‎webrtc-jni/src/main/cpp/src/JNI_AudioProcessing.cpp‎

Lines changed: 63 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,9 @@
3535
#include "modules/audio_processing/include/audio_processing.h"
3636
#include "rtc_base/logging.h"
3737

38+
#include <algorithm>
39+
#include <memory>
40+
3841
JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_audio_AudioProcessing_applyConfig
3942
(JNIEnv * env, jobject caller, jobject config)
4043
{
@@ -67,120 +70,87 @@ JNIEXPORT jint JNICALL Java_dev_onvoid_webrtc_media_audio_AudioProcessing_getStr
6770
return apm->stream_delay_ms();
6871
}
6972

70-
JNIEXPORT jint JNICALL Java_dev_onvoid_webrtc_media_audio_AudioProcessing_processStream
71-
(JNIEnv * env, jobject caller, jbyteArray src, jobject inputConfig, jobject outputConfig, jbyteArray dest)
73+
// Shared by processStream and processReverseStream, which differ only in the
74+
// direction they feed the audio processing module.
75+
static jint processFrame(JNIEnv * env, jobject caller, jbyteArray src, jobject inputConfig,
76+
jobject outputConfig, jbyteArray dest, bool reverse)
7277
{
73-
webrtc::AudioProcessing * apm = GetHandle<webrtc::AudioProcessing>(env, caller);
74-
CHECK_HANDLEV(apm, 0);
75-
76-
webrtc::StreamConfig srcConfig = jni::AudioProcessingStreamConfig::toNative(env, jni::JavaLocalRef<jobject>(env, inputConfig));
77-
webrtc::StreamConfig dstConfig = jni::AudioProcessingStreamConfig::toNative(env, jni::JavaLocalRef<jobject>(env, outputConfig));
78-
79-
jboolean isDstCopy = JNI_FALSE;
80-
81-
jbyte * srcPtr = env->GetByteArrayElements(src, NULL);
82-
jbyte * dstPtr = env->GetByteArrayElements(dest, &isDstCopy);
83-
84-
const int16_t * srcFrame = reinterpret_cast<const int16_t *>(srcPtr);
85-
int16_t * dstFrame = reinterpret_cast<int16_t *>(dstPtr);
78+
try {
79+
webrtc::AudioProcessing * apm = GetHandle<webrtc::AudioProcessing>(env, caller);
80+
CHECK_HANDLEV(apm, 0);
8681

87-
int result;
82+
webrtc::StreamConfig srcConfig = jni::AudioProcessingStreamConfig::toNative(env, jni::JavaLocalRef<jobject>(env, inputConfig));
83+
webrtc::StreamConfig dstConfig = jni::AudioProcessingStreamConfig::toNative(env, jni::JavaLocalRef<jobject>(env, outputConfig));
8884

89-
if (srcConfig.num_channels() == 1 && dstConfig.num_channels() == 2) {
90-
// Up-mixing, only mono to stereo.
85+
// Up-mixing, only mono to stereo, builds the stereo input in dest.
9186
// For complex channel layouts a channel mixer is required.
92-
87+
const bool upMix = srcConfig.num_channels() == 1 && dstConfig.num_channels() == 2;
9388
const size_t srcNumSamples = srcConfig.num_samples();
9489
const size_t dstNumChannels = dstConfig.num_channels();
95-
const size_t frameSize = srcNumSamples * dstNumChannels;
90+
const size_t dstNumSamples = upMix
91+
? std::max(srcNumSamples * dstNumChannels, dstConfig.num_samples())
92+
: dstConfig.num_samples();
9693

97-
if (frameSize > webrtc::AudioFrame::kMaxDataSizeSamples) {
98-
return -9;
94+
if (upMix && srcNumSamples * dstNumChannels > webrtc::AudioFrame::kMaxDataSizeSamples) {
95+
return webrtc::AudioProcessing::kBadNumberChannelsError;
9996
}
100-
101-
for (int i = static_cast<int>(srcNumSamples) - 1; i >= 0; i--) {
102-
for (size_t j = 0; j < dstNumChannels; ++j) {
103-
dstFrame[dstNumChannels * i + j] = srcFrame[i];
104-
}
97+
if (static_cast<size_t>(env->GetArrayLength(src)) < srcNumSamples * sizeof(int16_t) ||
98+
static_cast<size_t>(env->GetArrayLength(dest)) < dstNumSamples * sizeof(int16_t)) {
99+
return webrtc::AudioProcessing::kBadDataLengthError;
105100
}
106101

107-
srcConfig.set_num_channels(dstNumChannels);
108-
109-
result = apm->ProcessStream(dstFrame, srcConfig, dstConfig, dstFrame);
110-
}
111-
else {
112-
// Will also down-mix if required, e.g. from stereo to mono.
113-
result = apm->ProcessStream(srcFrame, srcConfig, dstConfig, dstFrame);
114-
}
115-
116-
if (isDstCopy == JNI_TRUE) {
117-
jsize dstLength = env->GetArrayLength(dest);
118-
119-
env->SetByteArrayRegion(dest, 0, dstLength, dstPtr);
120-
}
121-
122-
env->ReleaseByteArrayElements(src, srcPtr, JNI_ABORT);
123-
env->ReleaseByteArrayElements(dest, dstPtr, JNI_ABORT);
124-
125-
return result;
126-
}
127-
128-
JNIEXPORT jint JNICALL Java_dev_onvoid_webrtc_media_audio_AudioProcessing_processReverseStream
129-
(JNIEnv * env, jobject caller, jbyteArray src, jobject inputConfig, jobject outputConfig, jbyteArray dest)
130-
{
131-
webrtc::AudioProcessing * apm = GetHandle<webrtc::AudioProcessing>(env, caller);
132-
CHECK_HANDLEV(apm, 0);
133-
134-
webrtc::StreamConfig srcConfig = jni::AudioProcessingStreamConfig::toNative(env, jni::JavaLocalRef<jobject>(env, inputConfig));
135-
webrtc::StreamConfig dstConfig = jni::AudioProcessingStreamConfig::toNative(env, jni::JavaLocalRef<jobject>(env, outputConfig));
136-
137-
jboolean isDstCopy = JNI_FALSE;
138-
139-
jbyte * srcPtr = env->GetByteArrayElements(src, nullptr);
140-
jbyte * dstPtr = env->GetByteArrayElements(dest, &isDstCopy);
141-
142-
const int16_t * srcFrame = reinterpret_cast<const int16_t *>(srcPtr);
143-
int16_t * dstFrame = reinterpret_cast<int16_t *>(dstPtr);
144-
145-
int result;
146-
147-
if (srcConfig.num_channels() == 1 && dstConfig.num_channels() == 2) {
148-
// Up-mixing, only mono to stereo.
149-
// For complex channel layouts a channel mixer is required.
150-
151-
const size_t srcNumSamples = srcConfig.num_samples();
152-
const size_t dstNumChannels = dstConfig.num_channels();
153-
const size_t frameSize = srcNumSamples * dstNumChannels;
102+
auto releaseSrc = [env, src](jbyte * bytes) {
103+
env->ReleaseByteArrayElements(src, bytes, JNI_ABORT);
104+
};
105+
std::unique_ptr<jbyte, decltype(releaseSrc)> srcPtr(env->GetByteArrayElements(src, nullptr), releaseSrc);
106+
if (!srcPtr) {
107+
return webrtc::AudioProcessing::kUnspecifiedError;
108+
}
154109

155-
if (frameSize > webrtc::AudioFrame::kMaxDataSizeSamples) {
156-
return -9;
110+
// Mode 0 writes the samples back when the VM handed out a copy.
111+
auto releaseDst = [env, dest](jbyte * bytes) {
112+
env->ReleaseByteArrayElements(dest, bytes, 0);
113+
};
114+
std::unique_ptr<jbyte, decltype(releaseDst)> dstPtr(env->GetByteArrayElements(dest, nullptr), releaseDst);
115+
if (!dstPtr) {
116+
return webrtc::AudioProcessing::kUnspecifiedError;
157117
}
158118

159-
for (int i = static_cast<int>(srcNumSamples) - 1; i >= 0; i--) {
160-
for (size_t j = 0; j < dstNumChannels; ++j) {
161-
dstFrame[dstNumChannels * i + j] = srcFrame[i];
119+
const int16_t * srcFrame = reinterpret_cast<const int16_t *>(srcPtr.get());
120+
int16_t * dstFrame = reinterpret_cast<int16_t *>(dstPtr.get());
121+
122+
if (upMix) {
123+
for (int i = static_cast<int>(srcNumSamples) - 1; i >= 0; i--) {
124+
for (size_t j = 0; j < dstNumChannels; ++j) {
125+
dstFrame[dstNumChannels * i + j] = srcFrame[i];
126+
}
162127
}
163-
}
164128

165-
srcConfig.set_num_channels(dstNumChannels);
129+
srcConfig.set_num_channels(dstNumChannels);
130+
srcFrame = dstFrame;
131+
}
166132

167-
result = apm->ProcessReverseStream(dstFrame, srcConfig, dstConfig, dstFrame);
168-
}
169-
else {
170133
// Will also down-mix if required, e.g. from stereo to mono.
171-
result = apm->ProcessReverseStream(srcFrame, srcConfig, dstConfig, dstFrame);
134+
return reverse
135+
? apm->ProcessReverseStream(srcFrame, srcConfig, dstConfig, dstFrame)
136+
: apm->ProcessStream(srcFrame, srcConfig, dstConfig, dstFrame);
172137
}
173-
174-
if (isDstCopy == JNI_TRUE) {
175-
jsize dstLength = env->GetArrayLength(dest);
176-
177-
env->SetByteArrayRegion(dest, 0, dstLength, dstPtr);
138+
catch (...) {
139+
ThrowCxxJavaException(env);
140+
return webrtc::AudioProcessing::kUnspecifiedError;
178141
}
142+
}
179143

180-
env->ReleaseByteArrayElements(src, srcPtr, JNI_ABORT);
181-
env->ReleaseByteArrayElements(dest, dstPtr, JNI_ABORT);
144+
JNIEXPORT jint JNICALL Java_dev_onvoid_webrtc_media_audio_AudioProcessing_processStream
145+
(JNIEnv * env, jobject caller, jbyteArray src, jobject inputConfig, jobject outputConfig, jbyteArray dest)
146+
{
147+
return processFrame(env, caller, src, inputConfig, outputConfig, dest, false);
148+
}
182149

183-
return result;
150+
JNIEXPORT jint JNICALL Java_dev_onvoid_webrtc_media_audio_AudioProcessing_processReverseStream
151+
(JNIEnv * env, jobject caller, jbyteArray src, jobject inputConfig, jobject outputConfig, jbyteArray dest)
152+
{
153+
return processFrame(env, caller, src, inputConfig, outputConfig, dest, true);
184154
}
185155

186156
JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_audio_AudioProcessing_dispose

‎webrtc-jni/src/main/cpp/src/JNI_AudioResampler.cpp‎

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222

2323
#include "common_audio/resampler/include/push_resampler.h"
2424

25+
#include <memory>
26+
2527
JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_audio_AudioResampler_dispose
2628
(JNIEnv * env, jobject caller)
2729
{
@@ -66,28 +68,31 @@ JNIEXPORT jint JNICALL Java_dev_onvoid_webrtc_media_audio_AudioResampler_resampl
6668
webrtc::PushResampler<int16_t> * resampler = GetHandle<webrtc::PushResampler<int16_t>>(env, caller);
6769
CHECK_HANDLEV(resampler, -1);
6870

69-
jboolean isDstCopy = JNI_FALSE;
71+
auto releaseSrc = [env, samplesIn](jbyte * bytes) {
72+
env->ReleaseByteArrayElements(samplesIn, bytes, JNI_ABORT);
73+
};
74+
std::unique_ptr<jbyte, decltype(releaseSrc)> srcPtr(env->GetByteArrayElements(samplesIn, nullptr), releaseSrc);
75+
if (!srcPtr) {
76+
return -1;
77+
}
7078

71-
jbyte * srcPtr = env->GetByteArrayElements(samplesIn, nullptr);
72-
jbyte * dstPtr = env->GetByteArrayElements(samplesOut, &isDstCopy);
79+
// Mode 0 writes the samples back when the VM handed out a copy.
80+
auto releaseDst = [env, samplesOut](jbyte * bytes) {
81+
env->ReleaseByteArrayElements(samplesOut, bytes, 0);
82+
};
83+
std::unique_ptr<jbyte, decltype(releaseDst)> dstPtr(env->GetByteArrayElements(samplesOut, nullptr), releaseDst);
84+
if (!dstPtr) {
85+
return -1;
86+
}
7387

74-
int16_t * src16Ptr = reinterpret_cast<int16_t *>(srcPtr);
75-
int16_t * dst16Ptr = reinterpret_cast<int16_t*>(dstPtr);
88+
int16_t * src16Ptr = reinterpret_cast<int16_t *>(srcPtr.get());
89+
int16_t * dst16Ptr = reinterpret_cast<int16_t*>(dstPtr.get());
7690

7791
webrtc::InterleavedView<const int16_t> src(src16Ptr, srcSamplesPerChannel, channels);
7892
webrtc::InterleavedView<int16_t> dst(dst16Ptr, dstSamplesPerChannel, channels);
7993

8094
resampler->Resample(src, dst);
8195

82-
if (isDstCopy == JNI_TRUE) {
83-
jsize dstLength = env->GetArrayLength(samplesOut);
84-
85-
env->SetByteArrayRegion(samplesOut, 0, dstLength, dstPtr);
86-
}
87-
88-
env->ReleaseByteArrayElements(samplesIn, srcPtr, JNI_ABORT);
89-
env->ReleaseByteArrayElements(samplesOut, dstPtr, JNI_ABORT);
90-
9196
return static_cast<jint>(dstSamplesPerChannel);
9297
}
9398
catch (...) {

‎webrtc-jni/src/main/cpp/src/JNI_VideoBufferConverter.cpp‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
#include "libyuv/convert.h"
2424
#include "libyuv/video_common.h"
2525

26+
#include <memory>
27+
2628
size_t CalcBufferSize(int width, int height, int fourCC) {
2729
size_t bufferSize = 0;
2830

@@ -79,8 +81,15 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_video_VideoBufferConverter_I
7981
const uint8_t * srcU = static_cast<uint8_t *>(env->GetDirectBufferAddress(jSrcU));
8082
const uint8_t * srcV = static_cast<uint8_t *>(env->GetDirectBufferAddress(jSrcV));
8183

82-
jbyte * arrayPtr = env->GetByteArrayElements(dst, nullptr);
83-
uint8_t * dstPtr = reinterpret_cast<uint8_t *>(arrayPtr);
84+
// Mode 0 writes the picture back when the VM handed out a copy.
85+
auto releaseArray = [env, dst](jbyte * bytes) {
86+
env->ReleaseByteArrayElements(dst, bytes, 0);
87+
};
88+
std::unique_ptr<jbyte, decltype(releaseArray)> arrayPtr(env->GetByteArrayElements(dst, nullptr), releaseArray);
89+
if (!arrayPtr) {
90+
return;
91+
}
92+
uint8_t * dstPtr = reinterpret_cast<uint8_t *>(arrayPtr.get());
8493

8594
const int conversionResult = libyuv::ConvertFromI420(srcY, srcStrideY, srcU, srcStrideU,
8695
srcV, srcStrideV, dstPtr, 0, width, height, static_cast<uint32_t>(fourCC));
@@ -89,11 +98,6 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_video_VideoBufferConverter_I
8998
env->Throw(jni::JavaRuntimeException(env, "Failed to convert buffer to I420: %d",
9099
conversionResult));
91100
}
92-
else {
93-
env->SetByteArrayRegion(dst, 0, arrayLength, arrayPtr);
94-
}
95-
96-
env->ReleaseByteArrayElements(dst, arrayPtr, JNI_ABORT);
97101
}
98102
catch (...) {
99103
ThrowCxxJavaException(env);
@@ -151,8 +155,14 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_video_VideoBufferConverter_b
151155
uint8_t * dstU = static_cast<uint8_t *>(env->GetDirectBufferAddress(jDstU));
152156
uint8_t * dstV = static_cast<uint8_t *>(env->GetDirectBufferAddress(jDstV));
153157

154-
jbyte * arrayPtr = env->GetByteArrayElements(src, nullptr);
155-
const uint8_t * srcPtr = reinterpret_cast<uint8_t *>(arrayPtr);
158+
auto releaseArray = [env, src](jbyte * bytes) {
159+
env->ReleaseByteArrayElements(src, bytes, JNI_ABORT);
160+
};
161+
std::unique_ptr<jbyte, decltype(releaseArray)> arrayPtr(env->GetByteArrayElements(src, nullptr), releaseArray);
162+
if (!arrayPtr) {
163+
return;
164+
}
165+
const uint8_t * srcPtr = reinterpret_cast<uint8_t *>(arrayPtr.get());
156166

157167
const int conversionResult = libyuv::ConvertToI420(
158168
srcPtr, arrayLength,
@@ -166,8 +176,6 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_video_VideoBufferConverter_b
166176
env->Throw(jni::JavaRuntimeException(env, "Failed to convert buffer to I420: %d",
167177
conversionResult));
168178
}
169-
170-
env->ReleaseByteArrayElements(src, arrayPtr, JNI_ABORT);
171179
}
172180
catch (...) {
173181
ThrowCxxJavaException(env);

‎webrtc/src/test/java/dev/onvoid/webrtc/media/audio/AudioProcessingTest.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,20 @@ void processReverseStream() {
252252
assertEquals(0, processReverse(audioProcessing, buffer));
253253
}
254254

255+
@Test
256+
void processShortBuffersRejected() {
257+
ProcessBuffer buffer = new ProcessBuffer(48000, 48000, 1, 2);
258+
buffer.dst = new byte[buffer.dst.length - 2];
259+
260+
assertEquals(-8, process(audioProcessing, buffer));
261+
assertEquals(-8, processReverse(audioProcessing, buffer));
262+
263+
buffer = new ProcessBuffer(48000, 48000, 2, 2);
264+
buffer.src = new byte[buffer.src.length - 2];
265+
266+
assertEquals(-8, process(audioProcessing, buffer));
267+
}
268+
255269
@Test
256270
void streamDelay() {
257271
int delay = 70;

0 commit comments

Comments
 (0)