Improve visibility of IMemory security risks
This change renames the IMemory raw pointer accessors to
unsecure*() to make it apparent to coders and code reviewers
that the returned buffer may potentially be shared with
untrusted processes, who may, after the fact, attempt to
read and/or modify the contents. This may lead to hard to
find security bugs and hopefully the rename makes it harder
to forget.
The change also attempts to fix all the callsites to make
everything build correctly, but in the processes, wherever the
callsite code was not obviously secure, I added a TODO requesting
the owners to either document why it's secure or to change the
code. Apologies in advance to the owners if there are some false
positives here - I don't have enough context to reason about all
the different callsites.
Test: Completely syntactic change. Made sure code still builds.
Change-Id: I5fb99aa797c488406083178a6b05355d98710d3b
diff --git a/media/codec2/sfplugin/Codec2Buffer.cpp b/media/codec2/sfplugin/Codec2Buffer.cpp
index 5c8ad56..b339a92 100644
--- a/media/codec2/sfplugin/Codec2Buffer.cpp
+++ b/media/codec2/sfplugin/Codec2Buffer.cpp
@@ -764,7 +764,11 @@
const std::shared_ptr<C2LinearBlock> &block,
const sp<IMemory> &memory,
int32_t heapSeqNum)
- : Codec2Buffer(format, new ABuffer(memory->pointer(), memory->size())),
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ : Codec2Buffer(format, new ABuffer(memory->unsecurePointer(), memory->size())),
mBlock(block),
mMemory(memory),
mHeapSeqNum(heapSeqNum) {
@@ -800,7 +804,7 @@
if (view.size() < length) {
return false;
}
- memcpy(view.data(), decrypted->pointer(), length);
+ memcpy(view.data(), decrypted->unsecurePointer(), length);
return true;
}
diff --git a/media/libaudioclient/AudioEffect.cpp b/media/libaudioclient/AudioEffect.cpp
index cf11936..1cc5fe6 100644
--- a/media/libaudioclient/AudioEffect.cpp
+++ b/media/libaudioclient/AudioEffect.cpp
@@ -159,7 +159,11 @@
mIEffect = iEffect;
mCblkMemory = cblk;
- mCblk = static_cast<effect_param_cblk_t*>(cblk->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ mCblk = static_cast<effect_param_cblk_t*>(cblk->unsecurePointer());
int bufOffset = ((sizeof(effect_param_cblk_t) - 1) / sizeof(int) + 1) * sizeof(int);
mCblk->buffer = (uint8_t *)mCblk + bufOffset;
diff --git a/media/libaudioclient/AudioRecord.cpp b/media/libaudioclient/AudioRecord.cpp
index a1b04ca..0f2d48e 100644
--- a/media/libaudioclient/AudioRecord.cpp
+++ b/media/libaudioclient/AudioRecord.cpp
@@ -759,7 +759,11 @@
status = NO_INIT;
goto exit;
}
- iMemPointer = output.cblk ->pointer();
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ iMemPointer = output.cblk ->unsecurePointer();
if (iMemPointer == NULL) {
ALOGE("%s(%d): Could not get control block pointer", __func__, mPortId);
status = NO_INIT;
@@ -774,7 +778,11 @@
if (output.buffers == 0) {
buffers = cblk + 1;
} else {
- buffers = output.buffers->pointer();
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ buffers = output.buffers->unsecurePointer();
if (buffers == NULL) {
ALOGE("%s(%d): Could not get buffer pointer", __func__, mPortId);
status = NO_INIT;
diff --git a/media/libaudioclient/AudioTrack.cpp b/media/libaudioclient/AudioTrack.cpp
index 4a80cd3..e8d7b60 100644
--- a/media/libaudioclient/AudioTrack.cpp
+++ b/media/libaudioclient/AudioTrack.cpp
@@ -406,7 +406,7 @@
mDoNotReconnect = doNotReconnect;
ALOGV_IF(sharedBuffer != 0, "%s(): sharedBuffer: %p, size: %zu",
- __func__, sharedBuffer->pointer(), sharedBuffer->size());
+ __func__, sharedBuffer->unsecurePointer(), sharedBuffer->size());
ALOGV("%s(): streamType %d frameCount %zu flags %04x",
__func__, streamType, frameCount, flags);
@@ -1508,7 +1508,11 @@
status = NO_INIT;
goto exit;
}
- void *iMemPointer = iMem->pointer();
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ void *iMemPointer = iMem->unsecurePointer();
if (iMemPointer == NULL) {
ALOGE("%s(%d): Could not get control block pointer", __func__, mPortId);
status = NO_INIT;
@@ -1563,7 +1567,11 @@
if (mSharedBuffer == 0) {
buffers = cblk + 1;
} else {
- buffers = mSharedBuffer->pointer();
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ buffers = mSharedBuffer->unsecurePointer();
if (buffers == NULL) {
ALOGE("%s(%d): Could not get buffer pointer", __func__, mPortId);
status = NO_INIT;
diff --git a/media/libaudioclient/IAudioTrack.cpp b/media/libaudioclient/IAudioTrack.cpp
index 83a568a..6219e7a 100644
--- a/media/libaudioclient/IAudioTrack.cpp
+++ b/media/libaudioclient/IAudioTrack.cpp
@@ -62,7 +62,7 @@
status_t status = remote()->transact(GET_CBLK, data, &reply);
if (status == NO_ERROR) {
cblk = interface_cast<IMemory>(reply.readStrongBinder());
- if (cblk != 0 && cblk->pointer() == NULL) {
+ if (cblk != 0 && cblk->unsecurePointer() == NULL) {
cblk.clear();
}
}
diff --git a/media/libaudioclient/IEffect.cpp b/media/libaudioclient/IEffect.cpp
index ce72dae..5d47dff 100644
--- a/media/libaudioclient/IEffect.cpp
+++ b/media/libaudioclient/IEffect.cpp
@@ -122,7 +122,7 @@
status_t status = remote()->transact(GET_CBLK, data, &reply);
if (status == NO_ERROR) {
cblk = interface_cast<IMemory>(reply.readStrongBinder());
- if (cblk != 0 && cblk->pointer() == NULL) {
+ if (cblk != 0 && cblk->unsecurePointer() == NULL) {
cblk.clear();
}
}
diff --git a/media/libaudioclient/include/media/IAudioFlinger.h b/media/libaudioclient/include/media/IAudioFlinger.h
index db09ddf..b580a88 100644
--- a/media/libaudioclient/include/media/IAudioFlinger.h
+++ b/media/libaudioclient/include/media/IAudioFlinger.h
@@ -70,8 +70,12 @@
return DEAD_OBJECT;
}
if (parcel->readInt32() != 0) {
+ // TODO: Using unsecurePointer() has some associated security
+ // pitfalls (see declaration for details).
+ // Either document why it is safe in this case or address
+ // the issue (e.g. by copying).
sharedBuffer = interface_cast<IMemory>(parcel->readStrongBinder());
- if (sharedBuffer == 0 || sharedBuffer->pointer() == NULL) {
+ if (sharedBuffer == 0 || sharedBuffer->unsecurePointer() == NULL) {
return BAD_VALUE;
}
}
@@ -269,13 +273,21 @@
(void)parcel->read(&inputId, sizeof(audio_io_handle_t));
if (parcel->readInt32() != 0) {
cblk = interface_cast<IMemory>(parcel->readStrongBinder());
- if (cblk == 0 || cblk->pointer() == NULL) {
+ // TODO: Using unsecurePointer() has some associated security
+ // pitfalls (see declaration for details).
+ // Either document why it is safe in this case or address
+ // the issue (e.g. by copying).
+ if (cblk == 0 || cblk->unsecurePointer() == NULL) {
return BAD_VALUE;
}
}
if (parcel->readInt32() != 0) {
buffers = interface_cast<IMemory>(parcel->readStrongBinder());
- if (buffers == 0 || buffers->pointer() == NULL) {
+ // TODO: Using unsecurePointer() has some associated security
+ // pitfalls (see declaration for details).
+ // Either document why it is safe in this case or address
+ // the issue (e.g. by copying).
+ if (buffers == 0 || buffers->unsecurePointer() == NULL) {
return BAD_VALUE;
}
}
diff --git a/media/libheif/HeifDecoderImpl.cpp b/media/libheif/HeifDecoderImpl.cpp
index bad4210..33ea1ca 100644
--- a/media/libheif/HeifDecoderImpl.cpp
+++ b/media/libheif/HeifDecoderImpl.cpp
@@ -173,7 +173,7 @@
// copy from cache if the request falls entirely in cache
if (offset + size <= mCachedOffset + mCachedSize) {
- memcpy(mMemory->pointer(), mCache.get() + offset - mCachedOffset, size);
+ memcpy(mMemory->unsecurePointer(), mCache.get() + offset - mCachedOffset, size);
return size;
}
@@ -271,7 +271,7 @@
if (bytesAvailable < (int64_t)size) {
size = bytesAvailable;
}
- memcpy(mMemory->pointer(), mCache.get() + offset - mCachedOffset, size);
+ memcpy(mMemory->unsecurePointer(), mCache.get() + offset - mCachedOffset, size);
return size;
}
@@ -360,12 +360,16 @@
sp<IMemory> sharedMem = mRetriever->getImageAtIndex(
-1, mOutputColor, true /*metaOnly*/);
- if (sharedMem == nullptr || sharedMem->pointer() == nullptr) {
+ if (sharedMem == nullptr || sharedMem->unsecurePointer() == nullptr) {
ALOGE("init: videoFrame is a nullptr");
return false;
}
- VideoFrame* videoFrame = static_cast<VideoFrame*>(sharedMem->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ VideoFrame* videoFrame = static_cast<VideoFrame*>(sharedMem->unsecurePointer());
ALOGV("Image dimension %dx%d, display %dx%d, angle %d, iccSize %d",
videoFrame->mWidth,
@@ -391,12 +395,17 @@
MediaSource::ReadOptions::SEEK_PREVIOUS_SYNC,
mOutputColor, true /*metaOnly*/);
- if (sharedMem == nullptr || sharedMem->pointer() == nullptr) {
+ if (sharedMem == nullptr || sharedMem->unsecurePointer() == nullptr) {
ALOGE("init: videoFrame is a nullptr");
return false;
}
- VideoFrame* videoFrame = static_cast<VideoFrame*>(sharedMem->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ VideoFrame* videoFrame = static_cast<VideoFrame*>(
+ sharedMem->unsecurePointer());
ALOGV("Sequence dimension %dx%d, display %dx%d, angle %d, iccSize %d",
videoFrame->mWidth,
@@ -487,7 +496,7 @@
{
Mutex::Autolock autolock(mLock);
- if (frameMemory == nullptr || frameMemory->pointer() == nullptr) {
+ if (frameMemory == nullptr || frameMemory->unsecurePointer() == nullptr) {
mAsyncDecodeDone = true;
mScanlineReady.signal();
break;
@@ -529,12 +538,16 @@
sp<IMemory> frameMemory = mRetriever->getImageRectAtIndex(
-1, mOutputColor, 0, 0, mImageInfo.mWidth, mSliceHeight);
- if (frameMemory == nullptr || frameMemory->pointer() == nullptr) {
+ if (frameMemory == nullptr || frameMemory->unsecurePointer() == nullptr) {
ALOGE("decode: metadata is a nullptr");
return false;
}
- VideoFrame* videoFrame = static_cast<VideoFrame*>(frameMemory->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ VideoFrame* videoFrame = static_cast<VideoFrame*>(frameMemory->unsecurePointer());
if (frameInfo != nullptr) {
initFrameInfo(frameInfo, videoFrame);
@@ -563,12 +576,16 @@
MediaSource::ReadOptions::SEEK_PREVIOUS_SYNC, mOutputColor);
}
- if (mFrameMemory == nullptr || mFrameMemory->pointer() == nullptr) {
+ if (mFrameMemory == nullptr || mFrameMemory->unsecurePointer() == nullptr) {
ALOGE("decode: videoFrame is a nullptr");
return false;
}
- VideoFrame* videoFrame = static_cast<VideoFrame*>(mFrameMemory->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ VideoFrame* videoFrame = static_cast<VideoFrame*>(mFrameMemory->unsecurePointer());
if (videoFrame->mSize == 0 ||
mFrameMemory->size() < videoFrame->getFlattenedSize()) {
ALOGE("decode: videoFrame size is invalid");
@@ -613,12 +630,16 @@
mTotalScanline = mSequenceInfo.mHeight;
mFrameMemory = mRetriever->getFrameAtIndex(frameIndex, mOutputColor);
- if (mFrameMemory == nullptr || mFrameMemory->pointer() == nullptr) {
+ if (mFrameMemory == nullptr || mFrameMemory->unsecurePointer() == nullptr) {
ALOGE("decode: videoFrame is a nullptr");
return false;
}
- VideoFrame* videoFrame = static_cast<VideoFrame*>(mFrameMemory->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ VideoFrame* videoFrame = static_cast<VideoFrame*>(mFrameMemory->unsecurePointer());
if (videoFrame->mSize == 0 ||
mFrameMemory->size() < videoFrame->getFlattenedSize()) {
ALOGE("decode: videoFrame size is invalid");
@@ -641,10 +662,14 @@
}
bool HeifDecoderImpl::getScanlineInner(uint8_t* dst) {
- if (mFrameMemory == nullptr || mFrameMemory->pointer() == nullptr) {
+ if (mFrameMemory == nullptr || mFrameMemory->unsecurePointer() == nullptr) {
return false;
}
- VideoFrame* videoFrame = static_cast<VideoFrame*>(mFrameMemory->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ VideoFrame* videoFrame = static_cast<VideoFrame*>(mFrameMemory->unsecurePointer());
uint8_t* src = videoFrame->getFlattenedData() + videoFrame->mRowBytes * mCurScanline++;
memcpy(dst, src, videoFrame->mBytesPerPixel * videoFrame->mWidth);
return true;
diff --git a/media/libmedia/IMediaHTTPConnection.cpp b/media/libmedia/IMediaHTTPConnection.cpp
index 1bb8d67..8cbb4c2 100644
--- a/media/libmedia/IMediaHTTPConnection.cpp
+++ b/media/libmedia/IMediaHTTPConnection.cpp
@@ -128,12 +128,12 @@
ALOGE("readAt got a NULL buffer");
return UNKNOWN_ERROR;
}
- if (mMemory->pointer() == NULL) {
- ALOGE("readAt got a NULL mMemory->pointer()");
+ if (mMemory->unsecurePointer() == NULL) {
+ ALOGE("readAt got a NULL mMemory->unsecurePointer()");
return UNKNOWN_ERROR;
}
- memcpy(buffer, mMemory->pointer(), len);
+ memcpy(buffer, mMemory->unsecurePointer(), len);
return len;
}
diff --git a/media/libmediaplayerservice/MetadataRetrieverClient.cpp b/media/libmediaplayerservice/MetadataRetrieverClient.cpp
index 4a3c65e..b44eb43 100644
--- a/media/libmediaplayerservice/MetadataRetrieverClient.cpp
+++ b/media/libmediaplayerservice/MetadataRetrieverClient.cpp
@@ -292,7 +292,7 @@
delete albumArt;
return NULL;
}
- MediaAlbumArt::init((MediaAlbumArt *) mAlbumArt->pointer(),
+ MediaAlbumArt::init((MediaAlbumArt *) mAlbumArt->unsecurePointer(),
albumArt->size(), albumArt->data());
delete albumArt; // We've taken our copy.
return mAlbumArt;
diff --git a/media/libmediaplayerservice/nuplayer/NuPlayerStreamListener.cpp b/media/libmediaplayerservice/nuplayer/NuPlayerStreamListener.cpp
index ee70306..b5142ed 100644
--- a/media/libmediaplayerservice/nuplayer/NuPlayerStreamListener.cpp
+++ b/media/libmediaplayerservice/nuplayer/NuPlayerStreamListener.cpp
@@ -154,7 +154,7 @@
}
memcpy(data,
- (const uint8_t *)mem->pointer()
+ (const uint8_t *)mem->unsecurePointer()
+ entry->mOffset,
copy);
diff --git a/media/libnblog/Reader.cpp b/media/libnblog/Reader.cpp
index f556e37..67d028d 100644
--- a/media/libnblog/Reader.cpp
+++ b/media/libnblog/Reader.cpp
@@ -45,7 +45,12 @@
}
Reader::Reader(const sp<IMemory>& iMemory, size_t size, const std::string &name)
- : Reader(iMemory != 0 ? (Shared *) iMemory->pointer() : NULL, size, name)
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ : Reader(iMemory != 0 ? (Shared *) iMemory->unsecurePointer() : NULL, size,
+ name)
{
mIMemory = iMemory;
}
@@ -156,7 +161,8 @@
bool Reader::isIMemory(const sp<IMemory>& iMemory) const
{
- return iMemory != 0 && mIMemory != 0 && iMemory->pointer() == mIMemory->pointer();
+ return iMemory != 0 && mIMemory != 0 &&
+ iMemory->unsecurePointer() == mIMemory->unsecurePointer();
}
// We make a set of the invalid types rather than the valid types when aligning
diff --git a/media/libnblog/Writer.cpp b/media/libnblog/Writer.cpp
index da3bd52..86d3b98 100644
--- a/media/libnblog/Writer.cpp
+++ b/media/libnblog/Writer.cpp
@@ -56,7 +56,11 @@
}
Writer::Writer(const sp<IMemory>& iMemory, size_t size)
- : Writer(iMemory != 0 ? (Shared *) iMemory->pointer() : NULL, size)
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ : Writer(iMemory != 0 ? (Shared *) iMemory->unsecurePointer() : NULL, size)
{
mIMemory = iMemory;
}
diff --git a/media/libstagefright/ACodecBufferChannel.cpp b/media/libstagefright/ACodecBufferChannel.cpp
index 266a240..dd6f7b4 100644
--- a/media/libstagefright/ACodecBufferChannel.cpp
+++ b/media/libstagefright/ACodecBufferChannel.cpp
@@ -153,7 +153,8 @@
}
if (destination.mType == ICrypto::kDestinationTypeSharedMemory) {
- memcpy(it->mCodecBuffer->base(), destination.mSharedMemory->pointer(), result);
+ memcpy(it->mCodecBuffer->base(),
+ destination.mSharedMemory->unsecurePointer(), result);
}
} else {
// Here we cast CryptoPlugin::SubSample to hardware::cas::native::V1_0::SubSample
@@ -219,7 +220,8 @@
if (dstBuffer.type == BufferType::SHARED_MEMORY) {
memcpy(it->mCodecBuffer->base(),
- (uint8_t*)it->mSharedEncryptedBuffer->pointer(), result);
+ (uint8_t*)it->mSharedEncryptedBuffer->unsecurePointer(),
+ result);
}
}
diff --git a/media/libstagefright/BufferImpl.cpp b/media/libstagefright/BufferImpl.cpp
index b760273..68440e6 100644
--- a/media/libstagefright/BufferImpl.cpp
+++ b/media/libstagefright/BufferImpl.cpp
@@ -32,7 +32,11 @@
// SharedMemoryBuffer
SharedMemoryBuffer::SharedMemoryBuffer(const sp<AMessage> &format, const sp<IMemory> &mem)
- : MediaCodecBuffer(format, new ABuffer(mem->pointer(), mem->size())),
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ : MediaCodecBuffer(format, new ABuffer(mem->unsecurePointer(), mem->size())),
mMemory(mem) {
}
diff --git a/media/libstagefright/CallbackDataSource.cpp b/media/libstagefright/CallbackDataSource.cpp
index 92e6eb9..2f8e6af 100644
--- a/media/libstagefright/CallbackDataSource.cpp
+++ b/media/libstagefright/CallbackDataSource.cpp
@@ -81,7 +81,8 @@
return ERROR_OUT_OF_RANGE;
}
CHECK(numRead >= 0 && (size_t)numRead <= bufferSize);
- memcpy(((uint8_t*)data) + totalNumRead, mMemory->pointer(), numRead);
+ memcpy(((uint8_t*)data) + totalNumRead, mMemory->unsecurePointer(),
+ numRead);
numLeft -= numRead;
totalNumRead += numRead;
}
diff --git a/media/libstagefright/CameraSource.cpp b/media/libstagefright/CameraSource.cpp
index 41f5db0..9b3f420 100644
--- a/media/libstagefright/CameraSource.cpp
+++ b/media/libstagefright/CameraSource.cpp
@@ -89,7 +89,7 @@
void CameraSourceListener::postData(int32_t msgType, const sp<IMemory> &dataPtr,
camera_frame_metadata_t * /* metadata */) {
ALOGV("postData(%d, ptr:%p, size:%zu)",
- msgType, dataPtr->pointer(), dataPtr->size());
+ msgType, dataPtr->unsecurePointer(), dataPtr->size());
sp<CameraSource> source = mSource.promote();
if (source.get() != NULL) {
@@ -966,8 +966,12 @@
// Check if frame contains a VideoNativeHandleMetadata.
if (frame->size() == sizeof(VideoNativeHandleMetadata)) {
- VideoNativeHandleMetadata *metadata =
- (VideoNativeHandleMetadata*)(frame->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ VideoNativeHandleMetadata *metadata =
+ (VideoNativeHandleMetadata*)(frame->unsecurePointer());
if (metadata->eType == kMetadataBufferTypeNativeHandleSource) {
handle = metadata->pHandle;
}
@@ -1047,7 +1051,7 @@
Mutex::Autolock autoLock(mLock);
for (List<sp<IMemory> >::iterator it = mFramesBeingEncoded.begin();
it != mFramesBeingEncoded.end(); ++it) {
- if ((*it)->pointer() == buffer->data()) {
+ if ((*it)->unsecurePointer() == buffer->data()) {
releaseOneRecordingFrame((*it));
mFramesBeingEncoded.erase(it);
++mNumFramesEncoded;
@@ -1102,7 +1106,11 @@
frameTime = *mFrameTimes.begin();
mFrameTimes.erase(mFrameTimes.begin());
mFramesBeingEncoded.push_back(frame);
- *buffer = new MediaBuffer(frame->pointer(), frame->size());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ *buffer = new MediaBuffer(frame->unsecurePointer(), frame->size());
(*buffer)->setObserver(this);
(*buffer)->add_ref();
(*buffer)->meta_data().setInt64(kKeyTime, frameTime);
@@ -1248,7 +1256,7 @@
mMemoryBases.erase(mMemoryBases.begin());
// Wrap native handle in sp<IMemory> so it can be pushed to mFramesReceived.
- VideoNativeHandleMetadata *metadata = (VideoNativeHandleMetadata*)(data->pointer());
+ VideoNativeHandleMetadata *metadata = (VideoNativeHandleMetadata*)(data->unsecurePointer());
metadata->eType = kMetadataBufferTypeNativeHandleSource;
metadata->pHandle = handle;
@@ -1296,7 +1304,11 @@
mMemoryBases.erase(mMemoryBases.begin());
// Wrap native handle in sp<IMemory> so it can be pushed to mFramesReceived.
- VideoNativeHandleMetadata *metadata = (VideoNativeHandleMetadata*)(data->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ VideoNativeHandleMetadata *metadata = (VideoNativeHandleMetadata*)(data->unsecurePointer());
metadata->eType = kMetadataBufferTypeNativeHandleSource;
metadata->pHandle = handle;
diff --git a/media/libstagefright/CameraSourceTimeLapse.cpp b/media/libstagefright/CameraSourceTimeLapse.cpp
index 2a819ad..e0a6eb3 100644
--- a/media/libstagefright/CameraSourceTimeLapse.cpp
+++ b/media/libstagefright/CameraSourceTimeLapse.cpp
@@ -245,11 +245,11 @@
ALOGV("createIMemoryCopy");
size_t source_size = source_data->size();
- void* source_pointer = source_data->pointer();
+ void* source_pointer = source_data->unsecurePointer();
sp<MemoryHeapBase> newMemoryHeap = new MemoryHeapBase(source_size);
sp<MemoryBase> newMemory = new MemoryBase(newMemoryHeap, 0, source_size);
- memcpy(newMemory->pointer(), source_pointer, source_size);
+ memcpy(newMemory->unsecurePointer(), source_pointer, source_size);
return newMemory;
}
diff --git a/media/libstagefright/FrameDecoder.cpp b/media/libstagefright/FrameDecoder.cpp
index f99dd1c..a5a07d6 100644
--- a/media/libstagefright/FrameDecoder.cpp
+++ b/media/libstagefright/FrameDecoder.cpp
@@ -99,7 +99,7 @@
ALOGE("not enough memory for VideoFrame size=%zu", size);
return NULL;
}
- VideoFrame* frameCopy = static_cast<VideoFrame*>(frameMem->pointer());
+ VideoFrame* frameCopy = static_cast<VideoFrame*>(frameMem->unsecurePointer());
frameCopy->init(frame, iccData, iccSize);
return frameMem;
@@ -206,7 +206,7 @@
// try to fill sequence meta's duration based on average frame rate,
// default to 33ms if frame rate is unavailable.
int32_t frameRate;
- VideoFrame* meta = static_cast<VideoFrame*>(metaMem->pointer());
+ VideoFrame* meta = static_cast<VideoFrame*>(metaMem->unsecurePointer());
if (trackMeta->findInt32(kKeyFrameRate, &frameRate) && frameRate > 0) {
meta->mDurationUs = 1000000LL / frameRate;
} else {
@@ -614,7 +614,7 @@
0,
dstBpp(),
mSurfaceControl != nullptr /*allocRotated*/);
- mFrame = static_cast<VideoFrame*>(frameMem->pointer());
+ mFrame = static_cast<VideoFrame*>(frameMem->unsecurePointer());
setFrame(frameMem);
}
@@ -895,7 +895,7 @@
if (mFrame == NULL) {
sp<IMemory> frameMem = allocVideoFrame(
trackMeta(), mWidth, mHeight, mTileWidth, mTileHeight, dstBpp());
- mFrame = static_cast<VideoFrame*>(frameMem->pointer());
+ mFrame = static_cast<VideoFrame*>(frameMem->unsecurePointer());
setFrame(frameMem);
}
diff --git a/media/libstagefright/StagefrightMediaScanner.cpp b/media/libstagefright/StagefrightMediaScanner.cpp
index cf4edae..ce73676 100644
--- a/media/libstagefright/StagefrightMediaScanner.cpp
+++ b/media/libstagefright/StagefrightMediaScanner.cpp
@@ -158,7 +158,11 @@
if (mRetriever->setDataSource(fd, 0, size) == OK) {
sp<IMemory> mem = mRetriever->extractAlbumArt();
if (mem != NULL) {
- MediaAlbumArt *art = static_cast<MediaAlbumArt *>(mem->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ MediaAlbumArt *art = static_cast<MediaAlbumArt *>(mem->unsecurePointer());
return art->clone();
}
}
diff --git a/media/libstagefright/foundation/MediaBuffer.cpp b/media/libstagefright/foundation/MediaBuffer.cpp
index 9beac05..8e245dc 100644
--- a/media/libstagefright/foundation/MediaBuffer.cpp
+++ b/media/libstagefright/foundation/MediaBuffer.cpp
@@ -72,7 +72,7 @@
}
} else {
getSharedControl()->clear();
- mData = (uint8_t *)mMemory->pointer() + sizeof(SharedControl);
+ mData = (uint8_t *)mMemory->unsecurePointer() + sizeof(SharedControl);
ALOGV("Allocated shared mem buffer of size %zu @ %p", size, mData);
}
}
diff --git a/media/libstagefright/foundation/MediaBufferGroup.cpp b/media/libstagefright/foundation/MediaBufferGroup.cpp
index 84ff9a6..3c25047 100644
--- a/media/libstagefright/foundation/MediaBufferGroup.cpp
+++ b/media/libstagefright/foundation/MediaBufferGroup.cpp
@@ -75,7 +75,7 @@
for (size_t i = 0; i < buffers; ++i) {
sp<IMemory> mem = memoryDealer->allocate(augmented_size);
- if (mem.get() == nullptr || mem->pointer() == nullptr) {
+ if (mem.get() == nullptr || mem->unsecurePointer() == nullptr) {
ALOGW("Only allocated %zu shared buffers of size %zu", i, buffer_size);
break;
}
diff --git a/media/libstagefright/include/media/stagefright/MediaBuffer.h b/media/libstagefright/include/media/stagefright/MediaBuffer.h
index ace63ae..9145b63 100644
--- a/media/libstagefright/include/media/stagefright/MediaBuffer.h
+++ b/media/libstagefright/include/media/stagefright/MediaBuffer.h
@@ -48,7 +48,11 @@
explicit MediaBuffer(const sp<ABuffer> &buffer);
#ifndef NO_IMEMORY
MediaBuffer(const sp<IMemory> &mem) :
- MediaBuffer((uint8_t *)mem->pointer() + sizeof(SharedControl), mem->size()) {
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ MediaBuffer((uint8_t *)mem->unsecurePointer() + sizeof(SharedControl), mem->size()) {
// delegate and override mMemory
mMemory = mem;
}
@@ -94,9 +98,13 @@
virtual int remoteRefcount() const {
#ifndef NO_IMEMORY
- if (mMemory.get() == nullptr || mMemory->pointer() == nullptr) return 0;
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ if (mMemory.get() == nullptr || mMemory->unsecurePointer() == nullptr) return 0;
int32_t remoteRefcount =
- reinterpret_cast<SharedControl *>(mMemory->pointer())->getRemoteRefcount();
+ reinterpret_cast<SharedControl *>(mMemory->unsecurePointer())->getRemoteRefcount();
// Sanity check so that remoteRefCount() is non-negative.
return remoteRefcount >= 0 ? remoteRefcount : 0; // do not allow corrupted data.
#else
@@ -107,8 +115,12 @@
// returns old value
int addRemoteRefcount(int32_t value) {
#ifndef NO_IMEMORY
- if (mMemory.get() == nullptr || mMemory->pointer() == nullptr) return 0;
- return reinterpret_cast<SharedControl *>(mMemory->pointer())->addRemoteRefcount(value);
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ if (mMemory.get() == nullptr || mMemory->unsecurePointer() == nullptr) return 0;
+ return reinterpret_cast<SharedControl *>(mMemory->unsecurePointer())->addRemoteRefcount(value);
#else
(void) value;
return 0;
@@ -121,8 +133,12 @@
static bool isDeadObject(const sp<IMemory> &memory) {
#ifndef NO_IMEMORY
- if (memory.get() == nullptr || memory->pointer() == nullptr) return false;
- return reinterpret_cast<SharedControl *>(memory->pointer())->isDeadObject();
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ if (memory.get() == nullptr || memory->unsecurePointer() == nullptr) return false;
+ return reinterpret_cast<SharedControl *>(memory->unsecurePointer())->isDeadObject();
#else
(void) memory;
return false;
@@ -220,7 +236,11 @@
inline SharedControl *getSharedControl() const {
#ifndef NO_IMEMORY
- return reinterpret_cast<SharedControl *>(mMemory->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ return reinterpret_cast<SharedControl *>(mMemory->unsecurePointer());
#else
return nullptr;
#endif
diff --git a/media/libstagefright/include/media/stagefright/RemoteDataSource.h b/media/libstagefright/include/media/stagefright/RemoteDataSource.h
index e191e6a..b634505 100644
--- a/media/libstagefright/include/media/stagefright/RemoteDataSource.h
+++ b/media/libstagefright/include/media/stagefright/RemoteDataSource.h
@@ -48,7 +48,7 @@
if (size > kBufferSize) {
size = kBufferSize;
}
- return mSource->readAt(offset, mMemory->pointer(), size);
+ return mSource->readAt(offset, mMemory->unsecurePointer(), size);
}
virtual status_t getSize(off64_t *size) {
return mSource->getSize(size);
diff --git a/media/libstagefright/omx/OMXNodeInstance.cpp b/media/libstagefright/omx/OMXNodeInstance.cpp
index ddb4ba0..e1c3916 100644
--- a/media/libstagefright/omx/OMXNodeInstance.cpp
+++ b/media/libstagefright/omx/OMXNodeInstance.cpp
@@ -128,7 +128,7 @@
}
OMX_U8 *getPointer() {
- return mMem.get() ? static_cast<OMX_U8*>(mMem->pointer()) :
+ return mMem.get() ? static_cast<OMX_U8*>(mMem->unsecurePointer()) :
mHidlMemory.get() ? static_cast<OMX_U8*>(
static_cast<void*>(mHidlMemory->getPointer())) : nullptr;
}
@@ -1173,7 +1173,11 @@
return BAD_VALUE;
}
if (params != NULL) {
- paramsPointer = params->pointer();
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ paramsPointer = params->unsecurePointer();
paramsSize = params->size();
} else if (hParams != NULL) {
paramsPointer = hParams->getPointer();
diff --git a/media/utils/ServiceUtilities.cpp b/media/utils/ServiceUtilities.cpp
index db13903..7fe871c 100644
--- a/media/utils/ServiceUtilities.cpp
+++ b/media/utils/ServiceUtilities.cpp
@@ -222,9 +222,9 @@
off_t size = lseek(heap->getHeapID(), 0, SEEK_END);
lseek(heap->getHeapID(), 0, SEEK_SET);
- if (iMemory->pointer() == NULL || size < (off_t)iMemory->size()) {
+ if (iMemory->unsecurePointer() == NULL || size < (off_t)iMemory->size()) {
ALOGE("%s check failed: pointer %p size %zu fd size %u",
- __FUNCTION__, iMemory->pointer(), iMemory->size(), (uint32_t)size);
+ __FUNCTION__, iMemory->unsecurePointer(), iMemory->size(), (uint32_t)size);
return BAD_VALUE;
}