From 1f25ea94bbb76c196f1bb3d143f46722f1c1f42e Mon Sep 17 00:00:00 2001 From: Jiawei Shao Date: Fri, 10 Dec 2021 02:05:28 +0000 Subject: [PATCH] dawn_wire: Directly use the data in deserialized buffer when possible This patch tries to use the data directly in deserialized buffer when the data is "data-only" and doesn't affect the control flow instead of allocating and copying into a temporary buffer. Due to the protection of TOCTOU attacks, currently we only set "wire_is_data_only" on the parameter "data" in Queue.WriteBuffer() and Queue.WriteTexture(). With this patch, the performance of dawn_perf_tests BufferUploadPerf.Run/*_WriteBuffer_BufferSize_* with "-w" will be greatly improved (~20%) when the upload buffer size is greater than 1MB. BUG=chromium:1266727 Change-Id: I7a9d54c9b505975235ee37aa72ee97f082ad3aa3 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/72063 Reviewed-by: Austin Eng Commit-Queue: Jiawei Shao --- dawn_wire.json | 4 +- docs/codegen.md | 1 + generator/templates/dawn_wire/WireCmd.cpp | 49 ++++++++++++++--------- 3 files changed, 34 insertions(+), 20 deletions(-) diff --git a/dawn_wire.json b/dawn_wire.json index 57425732ee..41abdd8a62 100644 --- a/dawn_wire.json +++ b/dawn_wire.json @@ -70,13 +70,13 @@ {"name": "queue id", "type": "ObjectId" }, {"name": "buffer id", "type": "ObjectId" }, {"name": "buffer offset", "type": "uint64_t"}, - {"name": "data", "type": "uint8_t", "annotation": "const*", "length": "size"}, + {"name": "data", "type": "uint8_t", "annotation": "const*", "length": "size", "wire_is_data_only": true}, {"name": "size", "type": "uint64_t"} ], "queue write texture": [ {"name": "queue id", "type": "ObjectId" }, {"name": "destination", "type": "image copy texture", "annotation": "const*"}, - {"name": "data", "type": "uint8_t", "annotation": "const*", "length": "data size"}, + {"name": "data", "type": "uint8_t", "annotation": "const*", "length": "data size", "wire_is_data_only": true}, {"name": "data size", "type": "uint64_t"}, {"name": "data layout", "type": "texture data layout", "annotation": "const*"}, {"name": "writeSize", "type": "extent 3D", "annotation": "const*"} diff --git a/docs/codegen.md b/docs/codegen.md index 75a208b2c5..e0cfd55b03 100644 --- a/docs/codegen.md +++ b/docs/codegen.md @@ -42,6 +42,7 @@ A **record** is a list of **record members**, each of which is a dictionary with - `"length"` (default to 1 if not set), a string. Defines length of the array pointed to for pointer arguments. If not set the length is implicitly 1 (so not an array), but otherwise it can be set to the name of another member in the same record that will contain the length of the array (this is heavily used in the `fooCount` `foos` pattern in the API). As a special case `"strlen"` can be used for `const char*` record members to denote that the length should be determined with `strlen`. - `"optional"` (default to false) a boolean that says whether this member is optional. Member records can be optional if they are pointers (otherwise dawn_wire will always try to dereference them), objects (otherwise dawn_wire will always try to encode their ID and crash), or if they have a `"default"` key. Optional pointers and objects will always default to `nullptr`. - `"default"` (optional) a number or string. If set the record member will use that value as default value. Depending on the member's category it can be a number, a string containing a number, or the name of an enum/bitmask value. + - `"wire_is_data_only"` (default to false) a boolean that says whether it is safe to directly return a pointer of this member that is pointing to a piece of memory in the transfer buffer into dawn_wire. To prevent TOCTOU attacks, by default in dawn_wire we must ensure every single value returned to dawn_native a copy of what's in the wire, so `"wire_is_data_only"` is set to true only when the member is data-only and don't impact control flow. **`"native"`**, doesn't have any other key. This is used to define native types that can be referenced by name in other things. diff --git a/generator/templates/dawn_wire/WireCmd.cpp b/generator/templates/dawn_wire/WireCmd.cpp index e6d95046f7..0eb1bd1aa3 100644 --- a/generator/templates/dawn_wire/WireCmd.cpp +++ b/generator/templates/dawn_wire/WireCmd.cpp @@ -373,26 +373,39 @@ const volatile {{member_transfer_type(member)}}* memberBuffer; WIRE_TRY(deserializeBuffer->ReadN(memberLength, &memberBuffer)); - {{as_cType(member.type.name)}}* copiedMembers; - WIRE_TRY(GetSpace(allocator, memberLength, &copiedMembers)); - record->{{memberName}} = copiedMembers; + //* For data-only members (e.g. "data" in WriteBuffer and WriteTexture), they are + //* not security sensitive so we can directly refer the data inside the transfer + //* buffer in dawn_native. For other members, as prevention of TOCTOU attacks is an + //* important feature of the wire, we must make sure every single value returned to + //* dawn_native must be a copy of what's in the wire. + {% if member.json_data["wire_is_data_only"] %} + record->{{memberName}} = + const_cast(memberBuffer); - {% if member.type.is_wire_transparent %} - //* memcpy is not allowed to copy from volatile objects. However, these arrays - //* are just used as plain data, and don't impact control flow. So if the - //* underlying data were changed while the copy was still executing, we would - //* get different data - but it wouldn't cause unexpected downstream effects. - memcpy( - copiedMembers, - const_cast(memberBuffer), - {{member_transfer_sizeof(member)}} * memberLength); {% else %} - //* This loop cannot overflow because it iterates up to |memberLength|. Even if - //* memberLength were the maximum integer value, |i| would become equal to it - //* just before exiting the loop, but not increment past or wrap around. - for (decltype(memberLength) i = 0; i < memberLength; ++i) { - {{deserialize_member(member, "memberBuffer[i]", "copiedMembers[i]")}} - } + {{as_cType(member.type.name)}}* copiedMembers; + WIRE_TRY(GetSpace(allocator, memberLength, &copiedMembers)); + record->{{memberName}} = copiedMembers; + + {% if member.type.is_wire_transparent %} + //* memcpy is not allowed to copy from volatile objects. However, these + //* arrays are just used as plain data, and don't impact control flow. So if + //* the underlying data were changed while the copy was still executing, we + //* would get different data - but it wouldn't cause unexpected downstream + //* effects. + memcpy( + copiedMembers, + const_cast(memberBuffer), + {{member_transfer_sizeof(member)}} * memberLength); + {% else %} + //* This loop cannot overflow because it iterates up to |memberLength|. Even + //* if memberLength were the maximum integer value, |i| would become equal + //* to it just before exiting the loop, but not increment past or wrap + //* around. + for (decltype(memberLength) i = 0; i < memberLength; ++i) { + {{deserialize_member(member, "memberBuffer[i]", "copiedMembers[i]")}} + } + {% endif %} {% endif %} } {% endfor %}