Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(256)

Issue 15841011: Implement off-the-wire validation scheme for enum types. (Closed)

Created:
7 years, 6 months ago by Tom Sepez
Modified:
7 years, 6 months ago
CC:
chromium-reviews, darin-cc_chromium.org, markusheintz_, erikwright+watch_chromium.org
Visibility:
Public.

Description

Implement off-the-wire validation scheme for emum types. This CL adds explicit IPC macros that can be used to ensure that the values being read off the wire are legitimate for the enum type. BUG=176110 R=jam@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203892

Patch Set 1 #

Patch Set 2 : Correct check in content_settings.h #

Total comments: 2

Patch Set 3 : Re-implement using explicit macros. #

Patch Set 4 : Add content settings as example, fix typos. #

Patch Set 5 : #

Total comments: 2

Patch Set 6 : Add one more comment. #

Patch Set 7 : Peform enum validation upon write for debug builds. #

Total comments: 2

Patch Set 8 : Add () around macro parameter. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+63 lines, -42 lines) Patch
M chrome/common/common_param_traits_macros.h View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/tools/ipclist/ipcfuzz.cc View 1 2 2 chunks +3 lines, -5 lines 0 comments Download
M ipc/ipc_message_macros.h View 1 2 3 3 chunks +17 lines, -4 lines 0 comments Download
M ipc/ipc_message_null_macros.h View 1 2 3 chunks +2 lines, -4 lines 0 comments Download
M ipc/param_traits_log_macros.h View 1 2 2 chunks +3 lines, -5 lines 0 comments Download
M ipc/param_traits_macros.h View 1 2 3 4 5 1 chunk +21 lines, -2 lines 0 comments Download
M ipc/param_traits_read_macros.h View 1 2 3 4 5 6 7 2 chunks +8 lines, -8 lines 0 comments Download
M ipc/param_traits_write_macros.h View 1 2 3 4 5 6 2 chunks +6 lines, -7 lines 0 comments Download
M ipc/struct_constructor_macros.h View 1 2 1 chunk +0 lines, -2 lines 0 comments Download
M ipc/struct_destructor_macros.h View 1 2 1 chunk +1 line, -3 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
Tom Sepez
Hi John, Please review. I know you've been opposed to schemes like this in the ...
7 years, 6 months ago (2013-05-30 18:21:42 UTC) #1
Wez
Drive-by: This looks pretty neat to me. :)
7 years, 6 months ago (2013-05-30 18:30:44 UTC) #2
jam
On 2013/05/30 18:21:42, Tom Sepez wrote: > Hi John, Please review. I know you've been ...
7 years, 6 months ago (2013-05-30 23:23:52 UTC) #3
Tom Sepez
Thanks. I do think this helps, our IPC reviewers have found a few places where ...
7 years, 6 months ago (2013-05-30 23:41:23 UTC) #4
Tom Sepez
One other thing I should mention is that down the road, we might consider making ...
7 years, 6 months ago (2013-05-30 23:53:05 UTC) #5
jam
On 2013/05/30 23:41:23, Tom Sepez wrote: > Thanks. I do think this helps, our IPC ...
7 years, 6 months ago (2013-05-31 00:43:54 UTC) #6
darin (slow to review)
Maybe this validation code should reside with the param traits for the enumeration? In some ...
7 years, 6 months ago (2013-05-31 07:11:37 UTC) #7
aedla
Nice work. https://codereview.chromium.org/15841011/diff/2001/ipc/ipc_message_macros.h File ipc/ipc_message_macros.h (right): https://codereview.chromium.org/15841011/diff/2001/ipc/ipc_message_macros.h#newcode148 ipc/ipc_message_macros.h:148: // IPC to perform validaion, enum authors ...
7 years, 6 months ago (2013-05-31 08:35:15 UTC) #8
aedla
7 years, 6 months ago (2013-05-31 13:26:07 UTC) #9
jam
On 2013/05/31 07:11:37, darin wrote: > Maybe this validation code should reside with the param ...
7 years, 6 months ago (2013-05-31 15:04:58 UTC) #10
Tom Sepez
I could be convinced to add a IPC_ENUM_TRAITS_CHECKED(type, MAXVALUE) that covers the most common case ...
7 years, 6 months ago (2013-05-31 16:20:32 UTC) #11
Tom Sepez
John, Darin - howzabout something along the lines of Patch Set 5. Thanks!
7 years, 6 months ago (2013-05-31 22:36:25 UTC) #12
jam
lgtm (deferring to Darin to see if this latest version is fine with him) https://codereview.chromium.org/15841011/diff/22001/ipc/param_traits_write_macros.h ...
7 years, 6 months ago (2013-05-31 23:00:17 UTC) #13
Tom Sepez
https://codereview.chromium.org/15841011/diff/22001/ipc/param_traits_write_macros.h File ipc/param_traits_write_macros.h (right): https://codereview.chromium.org/15841011/diff/22001/ipc/param_traits_write_macros.h#newcode32 ipc/param_traits_write_macros.h:32: #define IPC_ENUM_TRAITS_VALIDATE(enum_name, validation_expression) \ On 2013/05/31 23:00:17, jam wrote: ...
7 years, 6 months ago (2013-05-31 23:01:54 UTC) #14
Tom Sepez
Darin, please take another look. Thanks heaps.
7 years, 6 months ago (2013-06-03 17:46:07 UTC) #15
darin (slow to review)
LGTM https://codereview.chromium.org/15841011/diff/32001/ipc/param_traits_read_macros.h File ipc/param_traits_read_macros.h (right): https://codereview.chromium.org/15841011/diff/32001/ipc/param_traits_read_macros.h#newcode40 ipc/param_traits_read_macros.h:40: if (!validation_expression) \ nit: you might consider writing ...
7 years, 6 months ago (2013-06-03 17:52:53 UTC) #16
Tom Sepez
https://codereview.chromium.org/15841011/diff/32001/ipc/param_traits_read_macros.h File ipc/param_traits_read_macros.h (right): https://codereview.chromium.org/15841011/diff/32001/ipc/param_traits_read_macros.h#newcode40 ipc/param_traits_read_macros.h:40: if (!validation_expression) \ On 2013/06/03 17:52:53, darin wrote: > ...
7 years, 6 months ago (2013-06-03 18:33:00 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tsepez@chromium.org/15841011/43001
7 years, 6 months ago (2013-06-03 18:33:26 UTC) #18
commit-bot: I haz the power
7 years, 6 months ago (2013-06-04 07:20:33 UTC) #19
Message was sent while issue was closed.
Change committed as 203892

Powered by Google App Engine
This is Rietveld 408576698