From ea30826bf3c1bdb3df2628047758690527e025b7 Mon Sep 17 00:00:00 2001 From: Avichal Rakesh Date: Mon, 25 Mar 2024 21:59:11 +0000 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. Merged-In: Ic406634de6f22a290abb414e80a7747927368b68 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(); } } }