Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
563073002:
Pepper UDP socket: buffer received packets in the plugin process to improve performance. (Closed)
DescriptionPepper UDP socket: buffer received packets in the plugin process to improve performance.
For each bound UDP socket, the Pepper layer in the plugin process maintains a
buffer to store received packets, and informs the browser process of available
buffer slots. The browser process does recvfrom and pushes the results to the
plugin process when there are available slots.
BUG=None
TEST=None
Committed: https://crrev.com/024dba2f439804d135020c7e40dc5264c6a13182
Cr-Commit-Position: refs/heads/master@{#295184}
Patch Set 1 #Patch Set 2 : cleanup #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 28
Patch Set 5 : #Patch Set 6 : #Messages
Total messages: 15 (2 generated)
yzshen@chromium.org changed reviewers: + dmichael@chromium.org, tsepez@chromium.org
Hi, David and Tom. Would you please take a look? Thanks! David: everything. Tom: security review, especially ppapi_messages.h.
The messages themselves are OK, but it may take til next week before I have time to make sure about the rest. https://codereview.chromium.org/563073002/diff/40001/ppapi/proxy/udp_socket_r... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/563073002/diff/40001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:346: memcpy(output_buffer, data.c_str(), data.size()); The DCHECK that output_buffer is big enough to handle data.size() should also be present in release builds to prevent this ever overflowing.
Thanks Tom! https://codereview.chromium.org/563073002/diff/40001/ppapi/proxy/udp_socket_r... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/563073002/diff/40001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:346: memcpy(output_buffer, data.c_str(), data.size()); On 2014/09/12 01:28:14, Tom Sepez wrote: > The DCHECK that output_buffer is big enough to handle data.size() should also be > present in release builds to prevent this ever overflowing. This is a private method of the class, and all callers have ensured that num_bytes >= data.size(). Therefore, I think it is okay to just have a DCHECK(). I added a comment above the method declaration. Do you think that is okay? Thanks!
This is looking pretty great, Yuzhu, thanks! I see now why you want to use "null" to specify the IO thread; so you don't have to plumb it through so the registrar doesn't need the IPC task runner in HandleOnIOThread. https://codereview.chromium.org/563073002/diff/60001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/563073002/diff/60001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:407: remaining_recv_slots_--; nit, might be better to do: if (remaining_recv_slots_) --remaining_recv_slots_; rather than let it wrap around in release builds. https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... File ppapi/proxy/resource_reply_thread_registrar.cc (right): https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... ppapi/proxy/resource_reply_thread_registrar.cc:73: info.thread_map.erase(sequence_thread_iter); After this, do you want something like... if (info.thread_map.empty()) map_.erase(resource_iter); To clean the PP_Resource out of map_ if it has no messages registered anymore? (Note this assumes you move the io thread stuff out to a separate set; otherwise you can't safely remove PP_Resources from map_) https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... ppapi/proxy/resource_reply_thread_registrar.cc:77: if (info.io_thread_message_types.find(nested_msg.type()) != nit: could just use io_thread_message_types.count(nested_msg.type()) https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... File ppapi/proxy/resource_reply_thread_registrar.h (right): https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... ppapi/proxy/resource_reply_thread_registrar.h:70: std::set<uint32> io_thread_message_types; I think it's likely that messages intended for the IO thread will always be per-message-id, and not dependent on an individual PP_Resource. So it seems like you could have this set separate, and check it before looking in the ResourceMap. Might be simpler. https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:56: PpapiPluginMsg_UDPSocket_PushRecvResult::ID); Looking at this, it seems better to me to eliminate the PP_Resource argument (see comments in the registrar). As-is, the entry for that PP_Resource never would get removed. It would be better if there was only 1 per message-id so that the map doesn't grow unbounded. https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:296: recvfrom_addr_resource_); optional: You could make SetRecvFromOutput always just take the front buffer. That would cut down some of these parameters and in this method you could unconditionally push the data on the back. Might be a tiny bit simpler. https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:304: RunCallback(recvfrom_callback_, result); It might make sense to do the SetRecvFromOutput stuff inside a TrackedCallback completion task (see TrackedCallback::set_completion_task). As written, there's a subtle potential race condition. RunCallback will result on the callback being run on a different thread. There's a slim chance that the resource will be destroyed before that happens, in which case the callback will actually be aborted. If the plugin has also freed its buffer and addr out-param at the same time that it freed the resource, then you will write to memory that has been freed. (Your stuff is all OK if the assumption is that the plugin will keep its out-params around until the callback executes, which is probably an appropriate requirement. I'm not sure we ever specified which was expected. But it's also kind of nice to write the out-param on the correct thread just before invoking, rather than early). Note the completion task will have the proxy lock, so it can still use members like the queue safely.
Thanks David! Please take a look at my replies. I haven't done all the changes you suggested yet. I would like to make sure I understand correctly before making some of the changes. Thanks! https://codereview.chromium.org/563073002/diff/60001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/563073002/diff/60001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:407: remaining_recv_slots_--; On 2014/09/12 19:49:06, dmichael wrote: > nit, might be better to do: > if (remaining_recv_slots_) > --remaining_recv_slots_; > rather than let it wrap around in release builds. Good catch! I changed OnMsgRecvSlotAvailable() to make sure |remaining_recv_slots_| never exceed kPluginReceiveBufferSlots. With that change, remaining_recv_slots_ should never wrap around. If you think it is okay, I would like to leave it as a DCHECK. Because if we use if(remaining_recv_slots_), it seems like we expect it to happen in some cases and would like to tolerate it. IMO that is unnecessary. What do you think? https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... File ppapi/proxy/resource_reply_thread_registrar.cc (right): https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... ppapi/proxy/resource_reply_thread_registrar.cc:73: info.thread_map.erase(sequence_thread_iter); On 2014/09/12 19:49:07, dmichael wrote: > After this, do you want something like... > if (info.thread_map.empty()) > map_.erase(resource_iter); > > To clean the PP_Resource out of map_ if it has no messages registered anymore? > (Note this assumes you move the io thread stuff out to a separate set; otherwise > you can't safely remove PP_Resources from map_) When a plugin resource is destroyed, we call Unregister() in its destructor. So the resource won't stay in map_ forever. I didn't remove it here because if a resource calls Register(), it is very likely to call Register() again. By leaving the resource in map_, we save some insertion/removal. https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... ppapi/proxy/resource_reply_thread_registrar.cc:77: if (info.io_thread_message_types.find(nested_msg.type()) != On 2014/09/12 19:49:06, dmichael wrote: > nit: could just use io_thread_message_types.count(nested_msg.type()) Is it because there is a performance difference? (I don't know the answer.) I thought find() conveys our intention a little more clear, but I am fine changing it if you prefer the other way. https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... File ppapi/proxy/resource_reply_thread_registrar.h (right): https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... ppapi/proxy/resource_reply_thread_registrar.h:70: std::set<uint32> io_thread_message_types; On 2014/09/12 19:49:07, dmichael wrote: > I think it's likely that messages intended for the IO thread will always be > per-message-id, and not dependent on an individual PP_Resource. So it seems like > you could have this set separate, and check it before looking in the > ResourceMap. Might be simpler. I agree. I have a question related to this in udp_socket_resource_base.cc. I will make the change after the question is answered. Thanks! https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:56: PpapiPluginMsg_UDPSocket_PushRecvResult::ID); On 2014/09/12 19:49:07, dmichael wrote: > Looking at this, it seems better to me to eliminate the PP_Resource argument > (see comments in the registrar). As-is, the entry for that PP_Resource never > would get removed. It would be better if there was only 1 per message-id so that > the map doesn't grow unbounded. The entry will be removed when the plugin resource is destructed. I am fine removing the PP_Resource argument, just want to make sure I do it in the right way: Do you think we should move this HandleOnIOThread() call to an Init method in ResourceReplyThreadRegistrar (in order to avoid making this call multiple times)? One issue is that people reading this file probably won't realize that PushRecvResult is handled differently. What do you think? Thanks! https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:296: recvfrom_addr_resource_); On 2014/09/12 19:49:07, dmichael wrote: > optional: You could make SetRecvFromOutput always just take the front buffer. > That would cut down some of these parameters and in this method you could > unconditionally push the data on the back. Might be a tiny bit simpler. Yeah, I considered that but thought that the current way is probably a little more efficient. What do you think? I could change it if you prefer that way. https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:304: RunCallback(recvfrom_callback_, result); On 2014/09/12 19:49:07, dmichael wrote: > It might make sense to do the SetRecvFromOutput stuff inside a TrackedCallback > completion task (see TrackedCallback::set_completion_task). > > As written, there's a subtle potential race condition. RunCallback will result > on the callback being run on a different thread. There's a slim chance that the > resource will be destroyed before that happens, in which case the callback will > actually be aborted. If the plugin has also freed its buffer and addr out-param > at the same time that it freed the resource, then you will write to memory that > has been freed. At this point we have finished writing to the out params on line 295. We tested that the callback was pending on line 274. Because we hold the proxy lock, we can be sure that nothing has got between line 274 and 295 to cause the callback to be aborted. I think we assume that out params need to be valid until callback is run, unless we explicitly state that we don't touch them after the PPAPI call returns (even with a pending completion callback). But that is not the case here. > > (Your stuff is all OK if the assumption is that the plugin will keep its > out-params around until the callback executes, which is probably an appropriate > requirement. I'm not sure we ever specified which was expected. But it's also > kind of nice to write the out-param on the correct thread just before invoking, > rather than early). I am not sure I understand, why writing to the out params early is not desirable? > > Note the completion task will have the proxy lock, so it can still use members > like the queue safely.
https://codereview.chromium.org/563073002/diff/60001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/563073002/diff/60001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:407: remaining_recv_slots_--; On 2014/09/15 20:59:39, yzshen1 wrote: > On 2014/09/12 19:49:06, dmichael wrote: > > nit, might be better to do: > > if (remaining_recv_slots_) > > --remaining_recv_slots_; > > rather than let it wrap around in release builds. > > Good catch! I changed OnMsgRecvSlotAvailable() to make sure > |remaining_recv_slots_| never exceed kPluginReceiveBufferSlots. With that > change, remaining_recv_slots_ should never wrap around. > > If you think it is okay, I would like to leave it as a DCHECK. Because if we use > if(remaining_recv_slots_), it seems like we expect it to happen in some cases > and would like to tolerate it. IMO that is unnecessary. What do you think? I meant you could leave the DCHECK also. But the message we're getting is untrusted, and we should avoid letting the object get in to a state we don't expect. There are probably no actual big problems if remaining_recv_slots_ underflows, but it's still best to not let a compromised renderer or PNaCl plugin put us in a weird state. Does that make sense? https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... File ppapi/proxy/resource_reply_thread_registrar.cc (right): https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... ppapi/proxy/resource_reply_thread_registrar.cc:73: info.thread_map.erase(sequence_thread_iter); On 2014/09/15 20:59:39, yzshen1 wrote: > On 2014/09/12 19:49:07, dmichael wrote: > > After this, do you want something like... > > if (info.thread_map.empty()) > > map_.erase(resource_iter); > > > > To clean the PP_Resource out of map_ if it has no messages registered anymore? > > (Note this assumes you move the io thread stuff out to a separate set; > otherwise > > you can't safely remove PP_Resources from map_) > > When a plugin resource is destroyed, we call Unregister() in its destructor. So > the resource won't stay in map_ forever. > I didn't remove it here because if a resource calls Register(), it is very > likely to call Register() again. By leaving the resource in map_, we save some > insertion/removal. Okay, thanks for explaining. https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... ppapi/proxy/resource_reply_thread_registrar.cc:77: if (info.io_thread_message_types.find(nested_msg.type()) != On 2014/09/15 20:59:39, yzshen1 wrote: > On 2014/09/12 19:49:06, dmichael wrote: > > nit: could just use io_thread_message_types.count(nested_msg.type()) > > Is it because there is a performance difference? (I don't know the answer.) I > thought find() conveys our intention a little more clear, but I am fine changing > it if you prefer the other way. No performance difference; just shorter. I find count() easier to read personally, just because it's less text for my brain to parse. And using it this way is fairly idiomatic IMO. But in base/stl_util.h there's also ContainsKey, which might be most readable of all. It's totally minor; I usually use count(), but they're all fine to me. https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:56: PpapiPluginMsg_UDPSocket_PushRecvResult::ID); On 2014/09/15 20:59:39, yzshen1 wrote: > On 2014/09/12 19:49:07, dmichael wrote: > > Looking at this, it seems better to me to eliminate the PP_Resource argument > > (see comments in the registrar). As-is, the entry for that PP_Resource never > > would get removed. It would be better if there was only 1 per message-id so > that > > the map doesn't grow unbounded. > > The entry will be removed when the plugin resource is destructed. > > I am fine removing the PP_Resource argument, just want to make sure I do it in > the right way: > Do you think we should move this HandleOnIOThread() call to an Init method in > ResourceReplyThreadRegistrar (in order to avoid making this call multiple > times)? One issue is that people reading this file probably won't realize that > PushRecvResult is handled differently. > > What do you think? Thanks! > Right, I went through the same thought process. I think I prefer to have it here, even though it will get called unnecessarily. Pros: 1) As you say, it makes it more obvious to a reader what's going on. 2) Apps that never use UDP won't have to have that entry in the HandleOnIOThread map. Cons: 1) We make the call unnecessarily. I'm happy with either way. I would lean towards leaving it here, if I was doing it. https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:296: recvfrom_addr_resource_); On 2014/09/15 20:59:39, yzshen1 wrote: > On 2014/09/12 19:49:07, dmichael wrote: > > optional: You could make SetRecvFromOutput always just take the front buffer. > > That would cut down some of these parameters and in this method you could > > unconditionally push the data on the back. Might be a tiny bit simpler. > > Yeah, I considered that but thought that the current way is probably a little > more efficient. What do you think? I could change it if you prefer that way. > How is it more efficient? I don't think you would be forced in to an extra copy for the string (though you might end up using string::swap when you pop it out of the queue). I think the code might be shorter and easier to follow that way. Certainly the parameter list would be easier to read. So that's my preference, but if it doesn't improve things, that's fine. https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:304: RunCallback(recvfrom_callback_, result); On 2014/09/15 20:59:39, yzshen1 wrote: > On 2014/09/12 19:49:07, dmichael wrote: > > It might make sense to do the SetRecvFromOutput stuff inside a TrackedCallback > > completion task (see TrackedCallback::set_completion_task). > > > > As written, there's a subtle potential race condition. RunCallback will result > > on the callback being run on a different thread. There's a slim chance that > the > > resource will be destroyed before that happens, in which case the callback > will > > actually be aborted. If the plugin has also freed its buffer and addr > out-param > > at the same time that it freed the resource, then you will write to memory > that > > has been freed. > At this point we have finished writing to the out params on line 295. We tested > that the callback was pending on line 274. Because we hold the proxy lock, we > can be sure that nothing has got between line 274 and 295 to cause the callback > to be aborted. Right. The problem is that the callback might be waiting to be invoked on a different thread. In that case, the callback is actually run on a different thread some time after we actually tell the callback to "Run". In between, it's possible for the callback to be aborted, and it will actually report PP_ERROR_ABORTED when it runs. It's definitely an unusual corner case, and it's not too risky or anything. But it seems cleaner to write the output params: 1) Only on success 2) On the correct thread, just before invocation. > > I think we assume that out params need to be valid until callback is run, unless > we explicitly state that we don't touch them after the PPAPI call returns (even > with a pending completion callback). But that is not the case here. > Yes, you're right, I had it a little mixed up; I don't think there's a chance of writing to invalid memory here, since at the time you do the write, the resource must still be alive. > > > > (Your stuff is all OK if the assumption is that the plugin will keep its > > out-params around until the callback executes, which is probably an > appropriate > > requirement. I'm not sure we ever specified which was expected. But it's also > > kind of nice to write the out-param on the correct thread just before > invoking, > > rather than early). > > I am not sure I understand, why writing to the out params early is not > desirable? It's a minor point, but you could end up writing to the out-params even for an aborted callback. We test this in the FileIO tests, for example: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/tests/test_f... It should be relatively benign, but it might be surprising to plugins. I think even though it's minor, it's probably better to not have that kind of surprising effect if we can help it. > > > > > Note the completion task will have the proxy lock, so it can still use members > > like the queue safely. > >
Thanks, David! Please take another look. https://codereview.chromium.org/563073002/diff/60001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/563073002/diff/60001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:407: remaining_recv_slots_--; > I meant you could leave the DCHECK also. But the message we're getting is > untrusted, and we should avoid letting the object get in to a state we don't > expect. > There are probably no actual big problems if remaining_recv_slots_ > underflows, but it's still best to not let a compromised renderer or PNaCl > plugin put us in a weird state. > > Does that make sense? Agreed. That is why I made changes to OnMsgRecvSlotAvailable(), where we handle untrusted requests. OnRecvFromCompleted(), on the other hand, is a callback from our network layer (trusted). Therefore, I didn't change this DCHECK. https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... File ppapi/proxy/resource_reply_thread_registrar.cc (right): https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/resource_rep... ppapi/proxy/resource_reply_thread_registrar.cc:77: if (info.io_thread_message_types.find(nested_msg.type()) != On 2014/09/16 17:39:00, dmichael wrote: > On 2014/09/15 20:59:39, yzshen1 wrote: > > On 2014/09/12 19:49:06, dmichael wrote: > > > nit: could just use io_thread_message_types.count(nested_msg.type()) > > > > Is it because there is a performance difference? (I don't know the answer.) I > > thought find() conveys our intention a little more clear, but I am fine > changing > > it if you prefer the other way. > > No performance difference; just shorter. I find count() easier to read > personally, just because it's less text for my brain to parse. And using it this > way is fairly idiomatic IMO. But in base/stl_util.h there's also ContainsKey, > which might be most readable of all. It's totally minor; I usually use count(), > but they're all fine to me. Done. Changed to count(). https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:56: PpapiPluginMsg_UDPSocket_PushRecvResult::ID); On 2014/09/16 17:39:00, dmichael wrote: > On 2014/09/15 20:59:39, yzshen1 wrote: > > On 2014/09/12 19:49:07, dmichael wrote: > > > Looking at this, it seems better to me to eliminate the PP_Resource argument > > > (see comments in the registrar). As-is, the entry for that PP_Resource never > > > would get removed. It would be better if there was only 1 per message-id so > > that > > > the map doesn't grow unbounded. > > > > The entry will be removed when the plugin resource is destructed. > > > > I am fine removing the PP_Resource argument, just want to make sure I do it in > > the right way: > > Do you think we should move this HandleOnIOThread() call to an Init method in > > ResourceReplyThreadRegistrar (in order to avoid making this call multiple > > times)? One issue is that people reading this file probably won't realize that > > PushRecvResult is handled differently. > > > > What do you think? Thanks! > > > Right, I went through the same thought process. I think I prefer to have it > here, even though it will get called unnecessarily. > Pros: > 1) As you say, it makes it more obvious to a reader what's going on. > 2) Apps that never use UDP won't have to have that entry in the HandleOnIOThread > map. > Cons: > 1) We make the call unnecessarily. > > I'm happy with either way. I would lean towards leaving it here, if I was doing > it. Done. I followed your suggestion. Thanks! https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:296: recvfrom_addr_resource_); On 2014/09/16 17:39:00, dmichael wrote: > On 2014/09/15 20:59:39, yzshen1 wrote: > > On 2014/09/12 19:49:07, dmichael wrote: > > > optional: You could make SetRecvFromOutput always just take the front > buffer. > > > That would cut down some of these parameters and in this method you could > > > unconditionally push the data on the back. Might be a tiny bit simpler. > > > > Yeah, I considered that but thought that the current way is probably a little > > more efficient. What do you think? I could change it if you prefer that way. > > > How is it more efficient? I don't think you would be forced in to an extra copy > for the string (though you might end up using string::swap when you pop it out > of the queue). > > I think the code might be shorter and easier to follow that way. Certainly the > parameter list would be easier to read. So that's my preference, but if it > doesn't improve things, that's fine. Because currently we don't support non-const ref string as input parameter of this method (please see my TODO above), putting |data| into |recv_buffers_| requires one copy, and then we need another copy into the user-provided buffer. https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:304: RunCallback(recvfrom_callback_, result); On 2014/09/16 17:39:00, dmichael wrote: > On 2014/09/15 20:59:39, yzshen1 wrote: > > On 2014/09/12 19:49:07, dmichael wrote: > > > It might make sense to do the SetRecvFromOutput stuff inside a > TrackedCallback > > > completion task (see TrackedCallback::set_completion_task). > > > > > > As written, there's a subtle potential race condition. RunCallback will > result > > > on the callback being run on a different thread. There's a slim chance that > > the > > > resource will be destroyed before that happens, in which case the callback > > will > > > actually be aborted. If the plugin has also freed its buffer and addr > > out-param > > > at the same time that it freed the resource, then you will write to memory > > that > > > has been freed. > > At this point we have finished writing to the out params on line 295. We > tested > > that the callback was pending on line 274. Because we hold the proxy lock, we > > can be sure that nothing has got between line 274 and 295 to cause the > callback > > to be aborted. > Right. The problem is that the callback might be waiting to be invoked on a > different thread. In that case, the callback is actually run on a different > thread some time after we actually tell the callback to "Run". In between, it's > possible for the callback to be aborted, and it will actually report > PP_ERROR_ABORTED when it runs. > > It's definitely an unusual corner case, and it's not too risky or anything. But > it seems cleaner to write the output params: > 1) Only on success > 2) On the correct thread, just before invocation. > > > > > I think we assume that out params need to be valid until callback is run, > unless > > we explicitly state that we don't touch them after the PPAPI call returns > (even > > with a pending completion callback). But that is not the case here. > > > Yes, you're right, I had it a little mixed up; I don't think there's a chance of > writing to invalid memory here, since at the time you do the write, the resource > must still be alive. > > > > > > > > (Your stuff is all OK if the assumption is that the plugin will keep its > > > out-params around until the callback executes, which is probably an > > appropriate > > > requirement. I'm not sure we ever specified which was expected. But it's > also > > > kind of nice to write the out-param on the correct thread just before > > invoking, > > > rather than early). > > > > I am not sure I understand, why writing to the out params early is not > > desirable? > It's a minor point, but you could end up writing to the out-params even for an > aborted callback. We test this in the FileIO tests, for example: > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/tests/test_f... > > It should be relatively benign, but it might be surprising to plugins. I think > even though it's minor, it's probably better to not have that kind of surprising > effect if we can help it. > > > > > > > > > Note the completion task will have the proxy lock, so it can still use > members > > > like the queue safely. > > > > I see your point now. I agree that we may write into the out parameters but return aborted. Because we didn't mention that we leave the output untouched on failure, I think it is not too bad. If you think it is okay, I would like to leave this as it is. It is a little bit simpler and I think quite a lot of places we do the same thing. Is that okay? :)
lgtm https://codereview.chromium.org/563073002/diff/60001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/563073002/diff/60001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:407: remaining_recv_slots_--; On 2014/09/16 19:09:07, yzshen1 wrote: > > I meant you could leave the DCHECK also. But the message we're getting is > > untrusted, and we should avoid letting the object get in to a state we don't > > expect. > > There are probably no actual big problems if remaining_recv_slots_ > > underflows, but it's still best to not let a compromised renderer or PNaCl > > plugin put us in a weird state. > > > > Does that make sense? > > Agreed. That is why I made changes to OnMsgRecvSlotAvailable(), where we handle > untrusted requests. OnRecvFromCompleted(), on the other hand, is a callback from > our network layer (trusted). Therefore, I didn't change this DCHECK. Right, wasn't reading carefully enough. SGTM https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:296: recvfrom_addr_resource_); On 2014/09/16 19:09:07, yzshen1 wrote: > On 2014/09/16 17:39:00, dmichael wrote: > > On 2014/09/15 20:59:39, yzshen1 wrote: > > > On 2014/09/12 19:49:07, dmichael wrote: > > > > optional: You could make SetRecvFromOutput always just take the front > > buffer. > > > > That would cut down some of these parameters and in this method you could > > > > unconditionally push the data on the back. Might be a tiny bit simpler. > > > > > > Yeah, I considered that but thought that the current way is probably a > little > > > more efficient. What do you think? I could change it if you prefer that way. > > > > > How is it more efficient? I don't think you would be forced in to an extra > copy > > for the string (though you might end up using string::swap when you pop it out > > of the queue). > > > > I think the code might be shorter and easier to follow that way. Certainly the > > parameter list would be easier to read. So that's my preference, but if it > > doesn't improve things, that's fine. > > Because currently we don't support non-const ref string as input parameter of > this method (please see my TODO above), putting |data| into |recv_buffers_| > requires one copy, and then we need another copy into the user-provided buffer. > > Oh, right, in this one case you'd have an extra copy. Makes sense. https://codereview.chromium.org/563073002/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:304: RunCallback(recvfrom_callback_, result); On 2014/09/16 19:09:07, yzshen1 wrote: > On 2014/09/16 17:39:00, dmichael wrote: > > On 2014/09/15 20:59:39, yzshen1 wrote: > > > On 2014/09/12 19:49:07, dmichael wrote: > > > > It might make sense to do the SetRecvFromOutput stuff inside a > > TrackedCallback > > > > completion task (see TrackedCallback::set_completion_task). > > > > > > > > As written, there's a subtle potential race condition. RunCallback will > > result > > > > on the callback being run on a different thread. There's a slim chance > that > > > the > > > > resource will be destroyed before that happens, in which case the callback > > > will > > > > actually be aborted. If the plugin has also freed its buffer and addr > > > out-param > > > > at the same time that it freed the resource, then you will write to memory > > > that > > > > has been freed. > > > At this point we have finished writing to the out params on line 295. We > > tested > > > that the callback was pending on line 274. Because we hold the proxy lock, > we > > > can be sure that nothing has got between line 274 and 295 to cause the > > callback > > > to be aborted. > > Right. The problem is that the callback might be waiting to be invoked on a > > different thread. In that case, the callback is actually run on a different > > thread some time after we actually tell the callback to "Run". In between, > it's > > possible for the callback to be aborted, and it will actually report > > PP_ERROR_ABORTED when it runs. > > > > It's definitely an unusual corner case, and it's not too risky or anything. > But > > it seems cleaner to write the output params: > > 1) Only on success > > 2) On the correct thread, just before invocation. > > > > > > > > I think we assume that out params need to be valid until callback is run, > > unless > > > we explicitly state that we don't touch them after the PPAPI call returns > > (even > > > with a pending completion callback). But that is not the case here. > > > > > Yes, you're right, I had it a little mixed up; I don't think there's a chance > of > > writing to invalid memory here, since at the time you do the write, the > resource > > must still be alive. > > > > > > > > > > > > (Your stuff is all OK if the assumption is that the plugin will keep its > > > > out-params around until the callback executes, which is probably an > > > appropriate > > > > requirement. I'm not sure we ever specified which was expected. But it's > > also > > > > kind of nice to write the out-param on the correct thread just before > > > invoking, > > > > rather than early). > > > > > > I am not sure I understand, why writing to the out params early is not > > > desirable? > > It's a minor point, but you could end up writing to the out-params even for an > > aborted callback. We test this in the FileIO tests, for example: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/tests/test_f... > > > > It should be relatively benign, but it might be surprising to plugins. I think > > even though it's minor, it's probably better to not have that kind of > surprising > > effect if we can help it. > > > > > > > > > > > > > Note the completion task will have the proxy lock, so it can still use > > members > > > > like the queue safely. > > > > > > > > I see your point now. I agree that we may write into the out parameters but > return aborted. > Because we didn't mention that we leave the output untouched on failure, I think > it is not too bad. If you think it is okay, I would like to leave this as it is. > It is a little bit simpler and I think quite a lot of places we do the same > thing. Is that okay? :) > Yeah... if we were starting from scratch, I'd want to do it the FileIO way everywhere, but it's not really enough of a problem to go fix.
Thanks a lot, David! And friendly ping for Tom. On Tue, Sep 16, 2014 at 12:29 PM, <dmichael@chromium.org> wrote: > lgtm > > > > > https://codereview.chromium.org/563073002/diff/60001/ > content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc > File > content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc > (right): > > https://codereview.chromium.org/563073002/diff/60001/ > content/browser/renderer_host/pepper/pepper_udp_socket_ > message_filter.cc#newcode407 > content/browser/renderer_host/pepper/pepper_udp_socket_ > message_filter.cc:407: > remaining_recv_slots_--; > On 2014/09/16 19:09:07, yzshen1 wrote: > >> > I meant you could leave the DCHECK also. But the message we're >> > getting is > >> > untrusted, and we should avoid letting the object get in to a state >> > we don't > >> > expect. >> > There are probably no actual big problems if remaining_recv_slots_ >> > underflows, but it's still best to not let a compromised renderer or >> > PNaCl > >> > plugin put us in a weird state. >> > >> > Does that make sense? >> > > Agreed. That is why I made changes to OnMsgRecvSlotAvailable(), where >> > we handle > >> untrusted requests. OnRecvFromCompleted(), on the other hand, is a >> > callback from > >> our network layer (trusted). Therefore, I didn't change this DCHECK. >> > Right, wasn't reading carefully enough. SGTM > > https://codereview.chromium.org/563073002/diff/60001/ > ppapi/proxy/udp_socket_resource_base.cc > File ppapi/proxy/udp_socket_resource_base.cc (right): > > https://codereview.chromium.org/563073002/diff/60001/ > ppapi/proxy/udp_socket_resource_base.cc#newcode296 > ppapi/proxy/udp_socket_resource_base.cc:296: recvfrom_addr_resource_); > On 2014/09/16 19:09:07, yzshen1 wrote: > >> On 2014/09/16 17:39:00, dmichael wrote: >> > On 2014/09/15 20:59:39, yzshen1 wrote: >> > > On 2014/09/12 19:49:07, dmichael wrote: >> > > > optional: You could make SetRecvFromOutput always just take the >> > front > >> > buffer. >> > > > That would cut down some of these parameters and in this method >> > you could > >> > > > unconditionally push the data on the back. Might be a tiny bit >> > simpler. > >> > > >> > > Yeah, I considered that but thought that the current way is >> > probably a > >> little >> > > more efficient. What do you think? I could change it if you prefer >> > that way. > >> > > >> > How is it more efficient? I don't think you would be forced in to an >> > extra > >> copy >> > for the string (though you might end up using string::swap when you >> > pop it out > >> > of the queue). >> > >> > I think the code might be shorter and easier to follow that way. >> > Certainly the > >> > parameter list would be easier to read. So that's my preference, but >> > if it > >> > doesn't improve things, that's fine. >> > > Because currently we don't support non-const ref string as input >> > parameter of > >> this method (please see my TODO above), putting |data| into >> > |recv_buffers_| > >> requires one copy, and then we need another copy into the >> > user-provided buffer. > > > Oh, right, in this one case you'd have an extra copy. Makes sense. > > https://codereview.chromium.org/563073002/diff/60001/ > ppapi/proxy/udp_socket_resource_base.cc#newcode304 > ppapi/proxy/udp_socket_resource_base.cc:304: > RunCallback(recvfrom_callback_, result); > On 2014/09/16 19:09:07, yzshen1 wrote: > >> On 2014/09/16 17:39:00, dmichael wrote: >> > On 2014/09/15 20:59:39, yzshen1 wrote: >> > > On 2014/09/12 19:49:07, dmichael wrote: >> > > > It might make sense to do the SetRecvFromOutput stuff inside a >> > TrackedCallback >> > > > completion task (see TrackedCallback::set_completion_task). >> > > > >> > > > As written, there's a subtle potential race condition. >> > RunCallback will > >> > result >> > > > on the callback being run on a different thread. There's a slim >> > chance > >> that >> > > the >> > > > resource will be destroyed before that happens, in which case >> > the callback > >> > > will >> > > > actually be aborted. If the plugin has also freed its buffer and >> > addr > >> > > out-param >> > > > at the same time that it freed the resource, then you will write >> > to memory > >> > > that >> > > > has been freed. >> > > At this point we have finished writing to the out params on line >> > 295. We > >> > tested >> > > that the callback was pending on line 274. Because we hold the >> > proxy lock, > >> we >> > > can be sure that nothing has got between line 274 and 295 to cause >> > the > >> > callback >> > > to be aborted. >> > Right. The problem is that the callback might be waiting to be >> > invoked on a > >> > different thread. In that case, the callback is actually run on a >> > different > >> > thread some time after we actually tell the callback to "Run". In >> > between, > >> it's >> > possible for the callback to be aborted, and it will actually report >> > PP_ERROR_ABORTED when it runs. >> > >> > It's definitely an unusual corner case, and it's not too risky or >> > anything. > >> But >> > it seems cleaner to write the output params: >> > 1) Only on success >> > 2) On the correct thread, just before invocation. >> > >> > > >> > > I think we assume that out params need to be valid until callback >> > is run, > >> > unless >> > > we explicitly state that we don't touch them after the PPAPI call >> > returns > >> > (even >> > > with a pending completion callback). But that is not the case >> > here. > >> > > >> > Yes, you're right, I had it a little mixed up; I don't think there's >> > a chance > >> of >> > writing to invalid memory here, since at the time you do the write, >> > the > >> resource >> > must still be alive. >> > >> > >> > > > >> > > > (Your stuff is all OK if the assumption is that the plugin will >> > keep its > >> > > > out-params around until the callback executes, which is probably >> > an > >> > > appropriate >> > > > requirement. I'm not sure we ever specified which was expected. >> > But it's > >> > also >> > > > kind of nice to write the out-param on the correct thread just >> > before > >> > > invoking, >> > > > rather than early). >> > > >> > > I am not sure I understand, why writing to the out params early is >> > not > >> > > desirable? >> > It's a minor point, but you could end up writing to the out-params >> > even for an > >> > aborted callback. We test this in the FileIO tests, for example: >> > >> > > https://code.google.com/p/chromium/codesearch#chromium/ > src/ppapi/tests/test_file_io.cc&l=642 > >> > >> > It should be relatively benign, but it might be surprising to >> > plugins. I think > >> > even though it's minor, it's probably better to not have that kind >> > of > >> surprising >> > effect if we can help it. >> > >> > > >> > > > >> > > > Note the completion task will have the proxy lock, so it can >> > still use > >> > members >> > > > like the queue safely. >> > > >> > > >> > > I see your point now. I agree that we may write into the out >> > parameters but > >> return aborted. >> Because we didn't mention that we leave the output untouched on >> > failure, I think > >> it is not too bad. If you think it is okay, I would like to leave this >> > as it is. > >> It is a little bit simpler and I think quite a lot of places we do the >> > same > >> thing. Is that okay? :) >> > > Yeah... if we were starting from scratch, I'd want to do it the FileIO > way everywhere, but it's not really enough of a problem to go fix. > > https://codereview.chromium.org/563073002/ > -- Best regards, Yuzhu Shen. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
The CQ bit was checked by yzshen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/563073002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as 37c29a08fb5b447eba9086ab1a8a260bcdd3ba00
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/024dba2f439804d135020c7e40dc5264c6a13182 Cr-Commit-Position: refs/heads/master@{#295184} |
