<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi Jacopo, Hiro,<br>
</p>
<div class="moz-cite-prefix">On 9/27/21 7:20 PM, Jacopo Mondi wrote:<br>
</div>
<blockquote type="cite"
cite="mid:20210927135036.nr26sjb33xbz65ga@uno.localdomain">
<pre class="moz-quote-pre" wrap="">Hi Umang, Hiro,
On Fri, Sep 24, 2021 at 01:49:19PM +0530, Umang Jain wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Hi Hiro,
On 9/24/21 1:10 PM, Hirokazu Honda wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Hi Umang, thank you for the patch.
On Fri, Sep 24, 2021 at 3:44 PM Umang Jain <a class="moz-txt-link-rfc2396E" href="mailto:umang.jain@ideasonboard.com"><umang.jain@ideasonboard.com></a> wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">The Camera3RequestDescriptor containing the capture request
is adding to the descriptors_ map after a call to
<a class="moz-txt-link-freetext" href="CameraWorker::queueRequest()">CameraWorker::queueRequest()</a>. This is a race condition since
<a class="moz-txt-link-freetext" href="CameraWorker::queueRequest()">CameraWorker::queueRequest()</a> queues request to <a class="moz-txt-link-freetext" href="libcamera::Camera">libcamera::Camera</a>
asynchronously and the addition of the descriptor to the map
occurs with <a class="moz-txt-link-freetext" href="std::move()">std::move()</a>. Hence, it might happen that the async
queueRequest() hasn't finished but the descriptor gets <a class="moz-txt-link-freetext" href="std::move()ed">std::move()ed</a>.
Fix it by adding the descriptor to map first, before
<a class="moz-txt-link-freetext" href="CameraWorker::queueRequest()">CameraWorker::queueRequest()</a>.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I don't understand the problem well.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
It's a race between queueRequest() and adding to the descriptor_ <a class="moz-txt-link-freetext" href="std::map">std::map</a>.
queueRequest() is called asynchronously(separate thread) so it might happen
that queueRequest is still processing/queuing the request while we
<a class="moz-txt-link-freetext" href="std::move()ed">std::move()ed</a> the descriptor (Which wraps the request queueRequest is
accessing)
If we fix the order with correct data-access it fixes the issue.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">I think the pointer of descriptors.request_.get() is not invalidated
by <a class="moz-txt-link-freetext" href="std::move()">std::move()</a>.
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
You know, I'm not really sure :)
Let's try to find it out...
This is the descriptor lifecycle inside this function
int processRequest()
{
// Allocated on the stack, request_ is a unique_ptr<> embedded in
// descriptor
Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);
....
worker_.queueRequest(descriptor.request_.get());
descriptors_[descriptor.request_->cookie()] = <a class="moz-txt-link-freetext" href="std::move(descriptor)">std::move(descriptor)</a>;
}
<a class="moz-txt-link-freetext" href="Camera3RequestDescriptor::request_">Camera3RequestDescriptor::request_</a> is initialized as
request_ = <a class="moz-txt-link-freetext" href="std::make_unique">std::make_unique</a><CaptureRequest>(camera);
<a class="moz-txt-link-freetext" href="CameraDevice::descriptors_">CameraDevice::descriptors_</a> is a map of type:
<a class="moz-txt-link-freetext" href="std::map">std::map</a><uint64_t, Camera3RequestDescriptor> descriptors_;
And Camera3RequestDescriptor has a defaulted move assignment operator=()
Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;
As Camera3RequestDescriptor member are:
struct Camera3RequestDescriptor {
uint32_t frameNumber_ = 0;
<a class="moz-txt-link-freetext" href="std::vector">std::vector</a><camera3_stream_buffer_t> buffers_;
<a class="moz-txt-link-freetext" href="std::vector">std::vector</a><<a class="moz-txt-link-freetext" href="std::unique_ptr">std::unique_ptr</a><a class="moz-txt-link-rfc2396E" href="libcamera::FrameBuffer"><libcamera::FrameBuffer></a>> frameBuffers_;
CameraMetadata settings_;
<a class="moz-txt-link-freetext" href="std::unique_ptr">std::unique_ptr</a><CaptureRequest> request_;
};
And most of them have a non-implicitly defined move assignment operator (vecors
and unique_ptr) I think we fall in the "Implicitly defined" section of [1]
-------------------------------------------------------------------------------
Implicitly-defined move assignment operator
If the implicitly-declared move assignment operator is neither deleted nor
trivial, it is defined (that is, a function body is generated and compiled) by
the compiler if odr-used or needed for constant evaluation (since C++14).
For union types, the implicitly-defined move assignment operator copies the
object representation (as by <a class="moz-txt-link-freetext" href="std::memmove">std::memmove</a>).
For non-union class types (class and struct), the move assignment operator
performs full member-wise move assignment of the object's direct bases and
immediate non-static members, in their declaration order, using built-in
assignment for the scalars, memberwise move-assignment for arrays, and move
assignment operator for class types (called non-virtually).
-------------------------------------------------------------------------------
Which means we're effectively calling <a class="moz-txt-link-freetext" href="std::unique_ptr::operator">std::unique_ptr::operator</a>[](&&) on
request_.
As <a class="moz-txt-link-freetext" href="std::unique_ptr::operator">std::unique_ptr::operator</a>[](&&other) is equivalent to
reset(other.release());
I guess what happens is that the location in memory of request_ doesn't change
but it's only its ownership that gets moved to the newly created entry in the
descriptors_ map.
Hence I think the code is safe the way it is, even if it might look suspicious
or fragile.</pre>
</blockquote>
<p><br>
</p>
<p>On a lighter note, looks don't matter ? :P </p>
<p>I think I'll slightly dis-agree that it is safe..<br>
</p>
<p>I understand that the ::move is not really copying to a different
location of memory, rather transfer of ownership happens, so
technically it might not cause any problems, since pointers are
still pointing to memory location they are supposed to point to
(possibly v2 version of this patch is based on this fact). But,
::move()ing while an async operation is in play on a memory
location does concern me if we look at it. </p>
<p>If we want to queueRequest() /before/ the descriptor is moved, I
think that should also happen with a lock, since,
descriptor.request_.get() is a descriptor access, no? It might
happen that the queueRequest() might also set something on the
CaptureRequest pointer, so essentially, we would still to lock the
entire async operation before firing it off, until it returns.</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:20210927135036.nr26sjb33xbz65ga@uno.localdomain">
<pre class="moz-quote-pre" wrap="">
I think it could be made 'safer' if we want to by:
- worker_.queueRequest(descriptor.request_.get());
-
+ CaptureRequest *req;
{
MutexLocker descriptorsLock(descriptorsMutex_);
+ unsigned long requestCookie = descriptor.request_->cookie();
- descriptors_[descriptor.request_->cookie()] = <a class="moz-txt-link-freetext" href="std::move(descriptor)">std::move(descriptor)</a>;
+ descriptors_[requestCookie] = <a class="moz-txt-link-freetext" href="std::move(descriptor)">std::move(descriptor)</a>;
+ req = descriptors_[requestCookie].request_.get();
}
+ worker_.queueRequest(req);
+
return 0;
}</pre>
</blockquote>
<p><br>
</p>
<p>Precisely, that intended goal of the patch is to queueRequest()
after the descriptor is moved to the map. <br>
</p>
<blockquote type="cite"
cite="mid:20210927135036.nr26sjb33xbz65ga@uno.localdomain">
<pre class="moz-quote-pre" wrap="">
But I didn't get if Umang hit any issue that lead him to send this patch.</pre>
</blockquote>
<p><br>
</p>
<p>Please mind that v2 of this patch is already on the list last
week, which might be of interest and further discussion. Also, I
didn't get any issues with it, it's something Laurent initially
spotted while code review [1]</p>
<p><span><span data-dobid="hdw">Excerpt</span></span> below :<br>
</p>
<pre>><i> > We have a race condition here, worker_.queueRequest() should go after
</i>><i> > adding the request to the queue. Could you fix it in a patch on top ?
</i>><i>
</i>><i> Do you mean the race condition is existing already, with the
</i>><i> descriptors_ map (that has been removed from this patch)?
</i>
Correct, it's already here.
><i> Yes, I can introduce a patch before this one, that fixes the race first
</i>><i> in the map itself. Is my understanding correct?
</i>
Sounds good to me. It should be a small patch.
</pre>
<p>[1]:
<a class="moz-txt-link-freetext" href="https://lists.libcamera.org/pipermail/libcamera-devel/2021-September/025004.html">https://lists.libcamera.org/pipermail/libcamera-devel/2021-September/025004.html</a><br>
</p>
<blockquote type="cite"
cite="mid:20210927135036.nr26sjb33xbz65ga@uno.localdomain">
<pre class="moz-quote-pre" wrap="">
[1] <a class="moz-txt-link-freetext" href="https://en.cppreference.com/w/cpp/language/move_assignment">https://en.cppreference.com/w/cpp/language/move_assignment</a>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
Why do you think so? :)
diff --git a/src/android/camera_device.cpp
b/src/android/camera_device.cpp
index a693dcbe..0de7050d 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1063,13 +1063,13 @@ int
<a class="moz-txt-link-freetext" href="CameraDevice::processCaptureRequest(camera3_capture_request_t">CameraDevice::processCaptureRequest(camera3_capture_request_t</a> *camera3Reques
state_ = <a class="moz-txt-link-freetext" href="State::Running">State::Running</a>;
}
- worker_.queueRequest(descriptor.request_.get());
-
{
MutexLocker descriptorsLock(descriptorsMutex_);
descriptors_[descriptor.request_->cookie()] = <a class="moz-txt-link-freetext" href="std::move(descriptor)">std::move(descriptor)</a>;
}
+ worker_.queueRequest(descriptor.request_.get());
+
is a segfaulted here (and expected no?).
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
For sure it does, you're now using 'descriptor' after it has been moved and it's
been left in unspecified state. As shown above <a class="moz-txt-link-freetext" href="std::unique_ptr::operator=(&&)">std::unique_ptr::operator=(&&)</a>
has been called on request_ which calls request_.release(), hence accessing it
with <a class="moz-txt-link-freetext" href="std::unique_ptr::get()">std::unique_ptr::get()</a> is undefined behaviour.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Signed-off-by: Umang Jain <a class="moz-txt-link-rfc2396E" href="mailto:umang.jain@ideasonboard.com"><umang.jain@ideasonboard.com></a>
---
src/android/camera_device.cpp | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 21844e51..c4c03d86 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1065,13 +1065,14 @@ int <a class="moz-txt-link-freetext" href="CameraDevice::processCaptureRequest(camera3_capture_request_t">CameraDevice::processCaptureRequest(camera3_capture_request_t</a> *camera3Reques
state_ = <a class="moz-txt-link-freetext" href="State::Running">State::Running</a>;
}
- worker_.queueRequest(descriptor.request_.get());
-
+ unsigned long requestCookie = descriptor.request_->cookie();
{
MutexLocker descriptorsLock(descriptorsMutex_);
- descriptors_[descriptor.request_->cookie()] = <a class="moz-txt-link-freetext" href="std::move(descriptor)">std::move(descriptor)</a>;
+ descriptors_[requestCookie] = <a class="moz-txt-link-freetext" href="std::move(descriptor)">std::move(descriptor)</a>;
}
+ worker_.queueRequest(descriptors_[requestCookie].request_.get());
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Accessing descriptors_ must be while holding descriptorsMutex_.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I am in two minds here,
We can lock it yes, but we are just reading from the map so, should be fine?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I think accessing descriptors_ should always happen while holding the
descriptorsMutex_ lock, as we extract nodes from the map in requestComplete()
which might invalidates iterators and re-points the internal pointers in the
map. I can't tell what happens if done concurrently with accessing the map, but
doesn't seem like a good idea.
Thanks
j
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
-Hiro
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
return 0;
}
--
2.31.1
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</body>
</html>