From 31437d0f3c5029dd43c975990c84d05fcc1d03bc Mon Sep 17 00:00:00 2001 From: Avichal Rakesh Date: Fri, 12 Jan 2024 17:17:27 -0800 Subject: [PATCH] ExternalCameraHAL: dup fd when creating AIDL NativeHandle AIDL's NativeHandle do not have a concept of unowned file descriptors. If a NativeHandle object is created with an fd, NativeHandle implicitly assumes ownership of the fd. When passing fds over binder, ExternalCameraHAL used makeToAidl which which accidentally transferred ownership to the AIDL objects. Additionally, NativeHandles close owned fds on destruction, which led to multiple closure of fences. This CL changes the logic to use dupToAidl to ensure that NativeHandle objects are given ownership of a duped fds and don't interfere with any of the fds used for internal bookkeeping. Bug: 313115623 Test: Verified by partner that ExternalCameraHAL no longer double closes fds. Change-Id: Ic406634de6f22a290abb414e80a7747927368b68 --- .../default/ExternalCameraDeviceSession.cpp | 24 ++++++++++++------- .../default/ExternalCameraOfflineSession.cpp | 6 ++--- camera/device/default/ExternalCameraUtils.cpp | 16 ++++--------- 3 files changed, 24 insertions(+), 22 deletions(-) diff --git a/camera/device/default/ExternalCameraDeviceSession.cpp b/camera/device/default/ExternalCameraDeviceSession.cpp index a6ec4c783e..126b782112 100644 --- a/camera/device/default/ExternalCameraDeviceSession.cpp +++ b/camera/device/default/ExternalCameraDeviceSession.cpp @@ -789,8 +789,10 @@ Status ExternalCameraDeviceSession::switchToOffline( outputBuffer.bufferId = buffer.bufferId; outputBuffer.status = BufferStatus::ERROR; if (buffer.acquireFence >= 0) { - outputBuffer.releaseFence.fds.resize(1); - outputBuffer.releaseFence.fds.at(0).set(buffer.acquireFence); + native_handle_t* handle = native_handle_create(/*numFds*/ 1, /*numInts*/ 0); + handle->data[0] = buffer.acquireFence; + outputBuffer.releaseFence = android::dupToAidl(handle); + native_handle_delete(handle); } } else { offlineBuffers.push_back(buffer); @@ -1768,8 +1770,10 @@ Status ExternalCameraDeviceSession::processCaptureRequestError( result.outputBuffers[i].bufferId = req->buffers[i].bufferId; result.outputBuffers[i].status = BufferStatus::ERROR; if (req->buffers[i].acquireFence >= 0) { - result.outputBuffers[i].releaseFence.fds.resize(1); - result.outputBuffers[i].releaseFence.fds.at(0).set(req->buffers[i].acquireFence); + native_handle_t* handle = native_handle_create(/*numFds*/ 1, /*numInts*/ 0); + handle->data[0] = req->buffers[i].acquireFence; + result.outputBuffers[i].releaseFence = android::dupToAidl(handle); + native_handle_delete(handle); } } @@ -1813,16 +1817,20 @@ Status ExternalCameraDeviceSession::processCaptureResult(std::shared_ptrbuffers[i].fenceTimeout) { result.outputBuffers[i].status = BufferStatus::ERROR; if (req->buffers[i].acquireFence >= 0) { - result.outputBuffers[i].releaseFence.fds.resize(1); - result.outputBuffers[i].releaseFence.fds.at(0).set(req->buffers[i].acquireFence); + native_handle_t* handle = native_handle_create(/*numFds*/ 1, /*numInts*/ 0); + handle->data[0] = req->buffers[i].acquireFence; + result.outputBuffers[i].releaseFence = android::dupToAidl(handle); + native_handle_delete(handle); } notifyError(req->frameNumber, req->buffers[i].streamId, ErrorCode::ERROR_BUFFER); } else { result.outputBuffers[i].status = BufferStatus::OK; // TODO: refactor if (req->buffers[i].acquireFence >= 0) { - result.outputBuffers[i].releaseFence.fds.resize(1); - result.outputBuffers[i].releaseFence.fds.at(0).set(req->buffers[i].acquireFence); + native_handle_t* handle = native_handle_create(/*numFds*/ 1, /*numInts*/ 0); + handle->data[0] = req->buffers[i].acquireFence; + result.outputBuffers[i].releaseFence = android::dupToAidl(handle); + native_handle_delete(handle); } } } diff --git a/camera/device/default/ExternalCameraOfflineSession.cpp b/camera/device/default/ExternalCameraOfflineSession.cpp index 53bd44f4fa..536fa47a61 100644 --- a/camera/device/default/ExternalCameraOfflineSession.cpp +++ b/camera/device/default/ExternalCameraOfflineSession.cpp @@ -110,7 +110,7 @@ Status ExternalCameraOfflineSession::processCaptureResult(std::shared_ptrbuffers[i].acquireFence >= 0) { native_handle_t* handle = native_handle_create(/*numFds*/ 1, /*numInts*/ 0); handle->data[0] = req->buffers[i].acquireFence; - result.outputBuffers[i].releaseFence = android::makeToAidl(handle); + result.outputBuffers[i].releaseFence = android::dupToAidl(handle); } notifyError(req->frameNumber, req->buffers[i].streamId, ErrorCode::ERROR_BUFFER); } else { @@ -119,7 +119,7 @@ Status ExternalCameraOfflineSession::processCaptureResult(std::shared_ptrbuffers[i].acquireFence >= 0) { native_handle_t* handle = native_handle_create(/*numFds*/ 1, /*numInts*/ 0); handle->data[0] = req->buffers[i].acquireFence; - outputBuffer.releaseFence = android::makeToAidl(handle); + outputBuffer.releaseFence = android::dupToAidl(handle); } } } @@ -247,7 +247,7 @@ Status ExternalCameraOfflineSession::processCaptureRequestError( if (req->buffers[i].acquireFence >= 0) { native_handle_t* handle = native_handle_create(/*numFds*/ 1, /*numInts*/ 0); handle->data[0] = req->buffers[i].acquireFence; - outputBuffer.releaseFence = makeToAidl(handle); + outputBuffer.releaseFence = dupToAidl(handle); } } diff --git a/camera/device/default/ExternalCameraUtils.cpp b/camera/device/default/ExternalCameraUtils.cpp index 30c216f209..2dc3c77ec8 100644 --- a/camera/device/default/ExternalCameraUtils.cpp +++ b/camera/device/default/ExternalCameraUtils.cpp @@ -750,18 +750,12 @@ Size getMaxThumbnailResolution(const common::V1_0::helper::CameraMetadata& chars void freeReleaseFences(std::vector& results) { for (auto& result : results) { - native_handle_t* inputReleaseFence = - ::android::makeFromAidl(result.inputBuffer.releaseFence); - if (inputReleaseFence != nullptr) { - native_handle_close(inputReleaseFence); - native_handle_delete(inputReleaseFence); - } + // NativeHandles free fd's on desctruction. Simply delete the objects! + result.inputBuffer.releaseFence.fds.clear(); // Implicitly closes fds + result.inputBuffer.releaseFence.ints.clear(); for (auto& buf : result.outputBuffers) { - native_handle_t* outReleaseFence = ::android::makeFromAidl(buf.releaseFence); - if (outReleaseFence != nullptr) { - native_handle_close(outReleaseFence); - native_handle_delete(outReleaseFence); - } + buf.releaseFence.fds.clear(); // Implicitly closes fds + buf.releaseFence.ints.clear(); } } }