Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2968003005:
Support Serializing and Deserializing RepeatedField / RepeatedPtrField in IPC::Message (Closed)
DescriptionSupport Serializing and Deserializing RepeatedField / RepeatedPtrField in IPC::Message
After this change, users can define a repeated field in protobuf,
message Message {
repeated <type> foo;
}
and use
GetParamSize(sizer, p.foo())
WriteParam(m, p.foo())
and
ReadParam(m, iter, r->mutable_foo())
to serialize and deserialize the field.
Serializing and deserializing a RepeatedField / RepeatedPtrField are needed in
change https://codereview.chromium.org/2964613002/.
BUG=650926
Review-Url: https://codereview.chromium.org/2968003005
Cr-Commit-Position: refs/heads/master@{#487593}
Committed: https://chromium.googlesource.com/chromium/src/+/84eef4fac4a6d97ac9dd0f05a8853f54116654ce
Patch Set 1 #
Total comments: 6
Patch Set 2 : Resolve review comments #Patch Set 3 : Add IsPickleSizeSufficient function #Patch Set 4 : Update IsPickleSizeSufficent to avoid GetSize #
Total comments: 6
Patch Set 5 : Resolve review comments #
Total comments: 10
Patch Set 6 : Resolve review comments #
Total comments: 2
Patch Set 7 : Resolve review comments #
Total comments: 2
Messages
Total messages: 99 (70 generated)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Support Serializing and Deserializing RepeatedField / RepeatedPtrField in
IPC::Message
After this change, users can define a repeated field in protobuf,
message Message {
repeated <type> foo;
}
and use
GetParamSize(sizer, p.foo())
WriteParam(m, p.foo())
and
ReadParam(m, iter, r->mutable_foo())
to serialize and deserialize the field.
Serializing and deserializing a RepeatedField / RepeatedPtrField are needed in
change https://codereview.chromium.org/2964613002/.
BUG=650926
==========
to
==========
Support Serializing and Deserializing RepeatedField / RepeatedPtrField in
IPC::Message
After this change, users can define a repeated field in protobuf,
message Message {
repeated <type> foo;
}
and use
GetParamSize(sizer, p.foo())
WriteParam(m, p.foo())
and
ReadParam(m, iter, r->mutable_foo())
to serialize and deserialize the field.
Serializing and deserializing a RepeatedField / RepeatedPtrField are needed in
change https://codereview.chromium.org/2964613002/.
BUG=650926
==========
zijiehe@chromium.org changed reviewers: + dcheng@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2968003005/diff/100001/ipc/ipc_message_repeat... File ipc/ipc_message_repeated_field_utils.h (right): https://codereview.chromium.org/2968003005/diff/100001/ipc/ipc_message_repeat... ipc/ipc_message_repeated_field_utils.h:23: template <template<class> class RepeatedFieldLike, class P> Is it important to include the template<class> here? I'm wondering if it would be simpler to just omit it. https://codereview.chromium.org/2968003005/diff/100001/ipc/ipc_message_repeat... ipc/ipc_message_repeated_field_utils.h:43: for (int i = 0; i < size; i++) { Nit: let's keep the original logic to do bounds checking and the reservation?
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2968003005/diff/100001/ipc/ipc_message_repeat... File ipc/ipc_message_repeated_field_utils.h (right): https://codereview.chromium.org/2968003005/diff/100001/ipc/ipc_message_repeat... ipc/ipc_message_repeated_field_utils.h:23: template <template<class> class RepeatedFieldLike, class P> On 2017/07/10 18:38:10, dcheng wrote: > Is it important to include the template<class> here? I'm wondering if it would > be simpler to just omit it. These two comments are conflict :) If we prefer to check the bounds and reserve beforehand, template parameter P is required. https://codereview.chromium.org/2968003005/diff/100001/ipc/ipc_message_repeat... ipc/ipc_message_repeated_field_utils.h:43: for (int i = 0; i < size; i++) { On 2017/07/10 18:38:10, dcheng wrote: > Nit: let's keep the original logic to do bounds checking and the reservation? Done.
https://codereview.chromium.org/2968003005/diff/100001/ipc/ipc_message_repeat... File ipc/ipc_message_repeated_field_utils.h (right): https://codereview.chromium.org/2968003005/diff/100001/ipc/ipc_message_repeat... ipc/ipc_message_repeated_field_utils.h:23: template <template<class> class RepeatedFieldLike, class P> On 2017/07/10 23:53:37, Hzj_jie wrote: > On 2017/07/10 18:38:10, dcheng wrote: > > Is it important to include the template<class> here? I'm wondering if it would > > be simpler to just omit it. > > These two comments are conflict :) > If we prefer to check the bounds and reserve beforehand, template parameter P is > required. Hmm. I guess I don't really care if we Reserve(). However, I feel like we might need the bounds check either way: it looks to me like https://cs.chromium.org/chromium/src/third_party/protobuf/src/google/protobuf... doesn't check for overflow, and could easily hit problems. And if we have to do that, we may as well Reserve(). (And yes, if we need the template parameter, we need it. But in that case, it would be named and used, so it's no longer unused)
https://codereview.chromium.org/2968003005/diff/100001/ipc/ipc_message_repeat... File ipc/ipc_message_repeated_field_utils.h (right): https://codereview.chromium.org/2968003005/diff/100001/ipc/ipc_message_repeat... ipc/ipc_message_repeated_field_utils.h:23: template <template<class> class RepeatedFieldLike, class P> On 2017/07/11 05:38:59, dcheng wrote: > On 2017/07/10 23:53:37, Hzj_jie wrote: > > On 2017/07/10 18:38:10, dcheng wrote: > > > Is it important to include the template<class> here? I'm wondering if it > would > > > be simpler to just omit it. > > > > These two comments are conflict :) > > If we prefer to check the bounds and reserve beforehand, template parameter P > is > > required. > > Hmm. I guess I don't really care if we Reserve(). However, I feel like we might > need the bounds check either way: it looks to me like > https://cs.chromium.org/chromium/src/third_party/protobuf/src/google/protobuf... > doesn't check for overflow, and could easily hit problems. > > And if we have to do that, we may as well Reserve(). > > (And yes, if we need the template parameter, we need it. But in that case, it > would be named and used, so it's no longer unused) Why cannot we use base::Pickle::payload_size() to check the boundary? I followed the example in std::vector<P>, but really do not understand the condition at (https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.h?rcl=d9917f026d85...).
On 2017/07/11 05:59:02, Hzj_jie wrote: > https://codereview.chromium.org/2968003005/diff/100001/ipc/ipc_message_repeat... > File ipc/ipc_message_repeated_field_utils.h (right): > > https://codereview.chromium.org/2968003005/diff/100001/ipc/ipc_message_repeat... > ipc/ipc_message_repeated_field_utils.h:23: template <template<class> class > RepeatedFieldLike, class P> > On 2017/07/11 05:38:59, dcheng wrote: > > On 2017/07/10 23:53:37, Hzj_jie wrote: > > > On 2017/07/10 18:38:10, dcheng wrote: > > > > Is it important to include the template<class> here? I'm wondering if it > > would > > > > be simpler to just omit it. > > > > > > These two comments are conflict :) > > > If we prefer to check the bounds and reserve beforehand, template parameter > P > > is > > > required. > > > > Hmm. I guess I don't really care if we Reserve(). However, I feel like we > might > > need the bounds check either way: it looks to me like > > > https://cs.chromium.org/chromium/src/third_party/protobuf/src/google/protobuf... > > doesn't check for overflow, and could easily hit problems. > > > > And if we have to do that, we may as well Reserve(). > > > > (And yes, if we need the template parameter, we need it. But in that case, it > > would be named and used, so it's no longer unused) > > Why cannot we use base::Pickle::payload_size() to check the boundary? I followed > the example in std::vector<P>, but really do not understand the condition at > (https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.h?rcl=d9917f026d85...). The concern here is that number_of_elements * sizeof(Element) might overflow. If that happens, we'll allocate too little memory and there will be a heap buffer overrun. A clever attacker can use this to overwrite vtables and gain code execution.
On 2017/07/11 18:14:24, dcheng wrote: > On 2017/07/11 05:59:02, Hzj_jie wrote: > > > https://codereview.chromium.org/2968003005/diff/100001/ipc/ipc_message_repeat... > > File ipc/ipc_message_repeated_field_utils.h (right): > > > > > https://codereview.chromium.org/2968003005/diff/100001/ipc/ipc_message_repeat... > > ipc/ipc_message_repeated_field_utils.h:23: template <template<class> class > > RepeatedFieldLike, class P> > > On 2017/07/11 05:38:59, dcheng wrote: > > > On 2017/07/10 23:53:37, Hzj_jie wrote: > > > > On 2017/07/10 18:38:10, dcheng wrote: > > > > > Is it important to include the template<class> here? I'm wondering if it > > > would > > > > > be simpler to just omit it. > > > > > > > > These two comments are conflict :) > > > > If we prefer to check the bounds and reserve beforehand, template > parameter > > P > > > is > > > > required. > > > > > > Hmm. I guess I don't really care if we Reserve(). However, I feel like we > > might > > > need the bounds check either way: it looks to me like > > > > > > https://cs.chromium.org/chromium/src/third_party/protobuf/src/google/protobuf... > > > doesn't check for overflow, and could easily hit problems. > > > > > > And if we have to do that, we may as well Reserve(). > > > > > > (And yes, if we need the template parameter, we need it. But in that case, > it > > > would be named and used, so it's no longer unused) > > > > Why cannot we use base::Pickle::payload_size() to check the boundary? I > followed > > the example in std::vector<P>, but really do not understand the condition at > > > (https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.h?rcl=d9917f026d85...). > > The concern here is that number_of_elements * sizeof(Element) might overflow. If > that happens, we'll allocate too little memory and there will be a heap buffer > overrun. A clever attacker can use this to overwrite vtables and gain code > execution. Emmm, I used to believe base::Pickle::payload_size() is reliable, but it seems not. (https://cs.chromium.org/chromium/src/base/pickle.cc?rcl=4721652f4038674729480...). I will update the logic to add a new CheckContainerLength() function to ensure every container can be guarded with the known protections.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
Code has been updated.
https://codereview.chromium.org/2968003005/diff/160001/ipc/ipc_message_utils.h File ipc/ipc_message_utils.h (right): https://codereview.chromium.org/2968003005/diff/160001/ipc/ipc_message_utils.... ipc/ipc_message_utils.h:126: static inline bool IsPickleSizeSufficient(const base::Pickle* m, OK, sorry. I think I probably didn't do a good job explaining =/ I think we assume that some code (such as the array operator new[], or things like calloc()) is written to be overflow safe. So we don't need to change the serializers for various STL types. This just needs to be implemented for proto repeated fields which should make things simpler. https://codereview.chromium.org/2968003005/diff/160001/ipc/ipc_message_utils.... ipc/ipc_message_utils.h:132: // Rejects if count * raw_size is overflow. See BUG 1006367 for details. So we have to spelunk a bit into proto details here. RepeatedPtrField always contains a void*, so we want to make sure that INT_MAX / sizeof(void*) > count Similarly, for RepeatedField<Element>, we can check INT_MAX > sizeof(Element). Does that make sense? https://codereview.chromium.org/2968003005/diff/160001/ipc/ipc_message_utils.... ipc/ipc_message_utils.h:138: if (static_cast<size_t>(count) > m->payload_size()) I think it's OK if we omit this check; the important part is to make sure that proto's internal buffer size calculation doesn't overflow when it does sizeof(Element) * new_size in https://cs.chromium.org/chromium/src/third_party/protobuf/src/google/protobuf... We don't actually need to check the other bit, as it will naturally fail when we run out of bytes to read in the pickle.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2968003005/diff/160001/ipc/ipc_message_utils.h File ipc/ipc_message_utils.h (right): https://codereview.chromium.org/2968003005/diff/160001/ipc/ipc_message_utils.... ipc/ipc_message_utils.h:126: static inline bool IsPickleSizeSufficient(const base::Pickle* m, On 2017/07/12 08:52:14, dcheng wrote: > OK, sorry. I think I probably didn't do a good job explaining =/ > > I think we assume that some code (such as the array operator new[], or things > like calloc()) is written to be overflow safe. So we don't need to change the > serializers for various STL types. This just needs to be implemented for proto > repeated fields which should make things simpler. This change has been reverted. Instead a bug http://crbug.com/741866 has been opened to track the issue. https://codereview.chromium.org/2968003005/diff/160001/ipc/ipc_message_utils.... ipc/ipc_message_utils.h:132: // Rejects if count * raw_size is overflow. See BUG 1006367 for details. On 2017/07/12 08:52:14, dcheng wrote: > So we have to spelunk a bit into proto details here. RepeatedPtrField always > contains a void*, so we want to make sure that INT_MAX / sizeof(void*) > count > > Similarly, for RepeatedField<Element>, we can check INT_MAX > sizeof(Element). > > Does that make sense? Acknowledged. https://codereview.chromium.org/2968003005/diff/160001/ipc/ipc_message_utils.... ipc/ipc_message_utils.h:138: if (static_cast<size_t>(count) > m->payload_size()) On 2017/07/12 08:52:15, dcheng wrote: > I think it's OK if we omit this check; the important part is to make sure that > proto's internal buffer size calculation doesn't overflow when it does > sizeof(Element) * new_size in > https://cs.chromium.org/chromium/src/third_party/protobuf/src/google/protobuf... > > We don't actually need to check the other bit, as it will naturally fail when we > run out of bytes to read in the pickle. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Any comments?
https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_fuzzing_tests.cc File ipc/ipc_fuzzing_tests.cc (left): https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_fuzzing_tests.... ipc/ipc_fuzzing_tests.cc:101: #endif Is it confirmed that this isn't a problem for the bots anymore? https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_fuzzing_tests.cc File ipc/ipc_fuzzing_tests.cc (right): https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_fuzzing_tests.... ipc/ipc_fuzzing_tests.cc:126: TEST(IPCMessageIntegrity, DISABLE_ReadVectorTooLarge3) { Nit: DISABLED https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_message_repeat... File ipc/ipc_message_repeated_field_utils_unittest.cc (right): https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_message_repeat... ipc/ipc_message_repeated_field_utils_unittest.cc:99: *message.add_messages() = message1; This works, but I feel like it might be clearer to write: auto* nested_message = message.add_messages(); nested_message->set_number(1000); nested_message = message.add_messages(); nested_message->set_number(10000); or even just: nested_message.add_messages()->set_number(1000); nested_message.add_messages()->set_number(10000); (Same below) The reason is that written this way, it's less clear if any state from message1 is inherited between adding the first and second elements. https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_message_repeat... ipc/ipc_message_repeated_field_utils_unittest.cc:121: TEST(IPCMessageRepeatedFieldUtilsTest, PartialEmptyRepeatedFieldShouldBeSerialized) { Nit: git cl format to fix formatting https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_message_repeat... ipc/ipc_message_repeated_field_utils_unittest.cc:187: DISALBED_InvalidPickleShouldNotCrashRepeatedFieldDeserialization2) { Nit: DISABLED
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_fuzzing_tests.cc File ipc/ipc_fuzzing_tests.cc (left): https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_fuzzing_tests.... ipc/ipc_fuzzing_tests.cc:101: #endif On 2017/07/13 22:25:07, dcheng wrote: > Is it confirmed that this isn't a problem for the bots anymore? Looks like I should not try to include this change in to the CL. https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_fuzzing_tests.cc File ipc/ipc_fuzzing_tests.cc (right): https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_fuzzing_tests.... ipc/ipc_fuzzing_tests.cc:126: TEST(IPCMessageIntegrity, DISABLE_ReadVectorTooLarge3) { On 2017/07/13 22:25:07, dcheng wrote: > Nit: DISABLED Done. https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_message_repeat... File ipc/ipc_message_repeated_field_utils_unittest.cc (right): https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_message_repeat... ipc/ipc_message_repeated_field_utils_unittest.cc:99: *message.add_messages() = message1; On 2017/07/13 22:25:07, dcheng wrote: > This works, but I feel like it might be clearer to write: > > auto* nested_message = message.add_messages(); > nested_message->set_number(1000); > nested_message = message.add_messages(); > nested_message->set_number(10000); > > or even just: > > nested_message.add_messages()->set_number(1000); > nested_message.add_messages()->set_number(10000); > > (Same below) > > The reason is that written this way, it's less clear if any state from message1 > is inherited between adding the first and second elements. Done. https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_message_repeat... ipc/ipc_message_repeated_field_utils_unittest.cc:121: TEST(IPCMessageRepeatedFieldUtilsTest, PartialEmptyRepeatedFieldShouldBeSerialized) { On 2017/07/13 22:25:07, dcheng wrote: > Nit: git cl format to fix formatting Done. https://codereview.chromium.org/2968003005/diff/180001/ipc/ipc_message_repeat... ipc/ipc_message_repeated_field_utils_unittest.cc:187: DISALBED_InvalidPickleShouldNotCrashRepeatedFieldDeserialization2) { On 2017/07/13 22:25:07, dcheng wrote: > Nit: DISABLED Done. I totally failed to write two "DISABLED" tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM https://codereview.chromium.org/2968003005/diff/200001/ipc/ipc_message_repeat... File ipc/ipc_message_repeated_field_utils.h (right): https://codereview.chromium.org/2968003005/diff/200001/ipc/ipc_message_repeat... ipc/ipc_message_repeated_field_utils.h:5: #ifndef IPC_IPC_MESSAGE_REPEATED_FIELD_UTILS_H_ Nit: I would personally call this file protobuf_utils (since it's less clear what repeated_field_utils refers to on a global basis)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2968003005/diff/200001/ipc/ipc_message_repeat... File ipc/ipc_message_repeated_field_utils.h (right): https://codereview.chromium.org/2968003005/diff/200001/ipc/ipc_message_repeat... ipc/ipc_message_repeated_field_utils.h:5: #ifndef IPC_IPC_MESSAGE_REPEATED_FIELD_UTILS_H_ On 2017/07/14 18:07:40, dcheng wrote: > Nit: I would personally call this file protobuf_utils (since it's less clear > what repeated_field_utils refers to on a global basis) Done.
zijiehe@chromium.org changed reviewers: + cpu@chromium.org, jialiul@chromium.org
Hi, Carlos and Jialiu, would you please have a look at this change? Carlos: //ipc/ Jialiu: //chrome/common/safe_browsing/
LGTM
zijiehe@chromium.org changed reviewers: + rockot@chromium.org - cpu@chromium.org
It looks like Carlos is busy. Ken, would you mind to have a look at the change under //ipc/ folder? This change simply moves the logic under //chrome/common/safe_browsing/protobuf_message_param_traits.h into //ipc/ipc_message_protobuf_utils.h with minor changes to support both RepeatedField / RepeatedPtrField.
lgtm
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2968003005/#ps220001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
zijiehe@chromium.org changed reviewers: + xyzzyz@chromium.org
Adam, would you mind to have a look at this change? I need your sign-off to depend on protobuf in //ipc/ folder.
zijiehe@chromium.org changed reviewers: + pkasting@chromium.org - xyzzyz@chromium.org
I cannot reach Adam today. Peter, would you mind to sign-off this change to let //ipc/ depend on protobuf? Thank you.
DEPS change LGTM
The CQ bit was checked by zijiehe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xyzzyz@chromium.org changed reviewers: + xyzzyz@chromium.org
LGTM https://codereview.chromium.org/2968003005/diff/220001/ipc/ipc_message_protob... File ipc/ipc_message_protobuf_utils.h (right): https://codereview.chromium.org/2968003005/diff/220001/ipc/ipc_message_protob... ipc/ipc_message_protobuf_utils.h:25: typedef RepeatedFieldLike param_type; Type names start with a capital letter and have a capital letter for each new word, with no underscores: MyExcitingClass, MyExcitingEnum. The names of all types — classes, structs, type aliases, enums, and type template parameters — have the same naming convention. https://google.github.io/styleguide/cppguide.html#Type_Names
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1500404683065540,
"parent_rev": "30f6310f7e47ff55d3e06e39992147d3533deea8", "commit_rev":
"84eef4fac4a6d97ac9dd0f05a8853f54116654ce"}
Message was sent while issue was closed.
Description was changed from
==========
Support Serializing and Deserializing RepeatedField / RepeatedPtrField in
IPC::Message
After this change, users can define a repeated field in protobuf,
message Message {
repeated <type> foo;
}
and use
GetParamSize(sizer, p.foo())
WriteParam(m, p.foo())
and
ReadParam(m, iter, r->mutable_foo())
to serialize and deserialize the field.
Serializing and deserializing a RepeatedField / RepeatedPtrField are needed in
change https://codereview.chromium.org/2964613002/.
BUG=650926
==========
to
==========
Support Serializing and Deserializing RepeatedField / RepeatedPtrField in
IPC::Message
After this change, users can define a repeated field in protobuf,
message Message {
repeated <type> foo;
}
and use
GetParamSize(sizer, p.foo())
WriteParam(m, p.foo())
and
ReadParam(m, iter, r->mutable_foo())
to serialize and deserialize the field.
Serializing and deserializing a RepeatedField / RepeatedPtrField are needed in
change https://codereview.chromium.org/2964613002/.
BUG=650926
Review-Url: https://codereview.chromium.org/2968003005
Cr-Commit-Position: refs/heads/master@{#487593}
Committed:
https://chromium.googlesource.com/chromium/src/+/84eef4fac4a6d97ac9dd0f05a885...
==========
Message was sent while issue was closed.
Committed patchset #7 (id:220001) as https://chromium.googlesource.com/chromium/src/+/84eef4fac4a6d97ac9dd0f05a885...
Message was sent while issue was closed.
https://codereview.chromium.org/2968003005/diff/220001/ipc/ipc_message_protob... File ipc/ipc_message_protobuf_utils.h (right): https://codereview.chromium.org/2968003005/diff/220001/ipc/ipc_message_protob... ipc/ipc_message_protobuf_utils.h:25: typedef RepeatedFieldLike param_type; On 2017/07/18 20:34:55, xyzzyz wrote: > Type names start with a capital letter and have a capital letter for each new > word, with no underscores: MyExcitingClass, MyExcitingEnum. > The names of all types — classes, structs, type aliases, enums, and type > template parameters — have the same naming convention. > > https://google.github.io/styleguide/cppguide.html#Type_Names > This is a requirement in IPC AFAICT.
Message was sent while issue was closed.
On Tue, Jul 18, 2017 at 1:46 PM, <zijiehe@chromium.org> wrote: > > https://codereview.chromium.org/2968003005/diff/220001/ > ipc/ipc_message_protobuf_utils.h > File ipc/ipc_message_protobuf_utils.h (right): > > https://codereview.chromium.org/2968003005/diff/220001/ > ipc/ipc_message_protobuf_utils.h#newcode25 > ipc/ipc_message_protobuf_utils.h:25: typedef RepeatedFieldLike > param_type; > On 2017/07/18 20:34:55, xyzzyz wrote: > > Type names start with a capital letter and have a capital letter for > each new > > word, with no underscores: MyExcitingClass, MyExcitingEnum. > > The names of all types — classes, structs, type aliases, enums, and > type > > template parameters — have the same naming convention. > > > > https://google.github.io/styleguide/cppguide.html#Type_Names > > > > This is a requirement in IPC AFAICT. > It's not a requirement, but it is a convention. Definitely an unfortunate convention, but an established one all the same. > > https://codereview.chromium.org/2968003005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
