commit 75f554d9739cb62a3fcdd0e8cafb9115cd853787
author Corentin Wallez <cwallez@chromium.org> Thu Jun 17 16:04:29 2021 +0000
committer Dawn LUCI CQ <dawn-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Jun 17 16:04:29 2021 +0000
tree 6fc2bc476822d660e0e56b1863b93520b677aed9
parent 36edf8d8503cfac4c7f75c8db8231096b3c73b9e

WireCmd: disallow optional members with length="otherMember"

This didn't make sense since these members can be specified to have no
data by setting the length to 0.

 - Prevent uses of this patten by adding an assert in WireCmd.cpp's
   generator.
 - Fix SetBindGroup dynamicOffset to not be optional but default to
   nullptr instead.

This issues would cause a read of uninitialized pointers becaus the
generator code looked like this:

  SetBindGroupCmd cmd;
  cmd.dynamicOffsetCount = record.dynamicOffsetCount; // 1
  bool has_dynamicOffset = record.has_dynamicOffsetl // false
  if (has_dynamicOffset) {
    cmd.dynamicOffsets = ...;
  }
  // Oh no! dynamicOffsets contains garbage even if dynamicOffsetCount
  // is set to 1. dawn_native will happily read it.

Bug: chromium:1220036
Change-Id: I5c468b639f671cef3be2fa64667a0bf114fc902b
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/54643
Auto-Submit: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Stephen White <senorblanco@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Stephen White <senorblanco@chromium.org>

dawn.json

diff --git a/dawn.json b/dawn.json
index 79e5f4e..4151b95 100644
--- a/dawn.json
+++ b/dawn.json
@@ -572,7 +572,7 @@
                     {"name": "group index", "type": "uint32_t"},
                     {"name": "group", "type": "bind group"},
                     {"name": "dynamic offset count", "type": "uint32_t", "default": "0"},
-                    {"name": "dynamic offsets", "type": "uint32_t", "annotation": "const*", "length": "dynamic offset count", "optional": true}
+                    {"name": "dynamic offsets", "type": "uint32_t", "annotation": "const*", "length": "dynamic offset count", "default": "nullptr"}
                 ]
             },
             {
@@ -1229,7 +1229,7 @@
                     {"name": "group index", "type": "uint32_t"},
                     {"name": "group", "type": "bind group"},
                     {"name": "dynamic offset count", "type": "uint32_t", "default": "0"},
-                    {"name": "dynamic offsets", "type": "uint32_t", "annotation": "const*", "length": "dynamic offset count", "optional": true}
+                    {"name": "dynamic offsets", "type": "uint32_t", "annotation": "const*", "length": "dynamic offset count", "default": "nullptr"}
                 ]
             },
             {
@@ -1393,7 +1393,7 @@
                     {"name": "group index", "type": "uint32_t"},
                     {"name": "group", "type": "bind group"},
                     {"name": "dynamic offset count", "type": "uint32_t", "default": "0"},
-                    {"name": "dynamic offsets", "type": "uint32_t", "annotation": "const*", "length": "dynamic offset count", "optional": true}
+                    {"name": "dynamic offsets", "type": "uint32_t", "annotation": "const*", "length": "dynamic offset count", "default": "nullptr"}
                 ]
             },
             {

generator/templates/dawn_wire/WireCmd.cpp

diff --git a/generator/templates/dawn_wire/WireCmd.cpp b/generator/templates/dawn_wire/WireCmd.cpp
index 4816ee1..2c37290 100644
--- a/generator/templates/dawn_wire/WireCmd.cpp
+++ b/generator/templates/dawn_wire/WireCmd.cpp
@@ -361,6 +361,10 @@
             {% set memberName = as_varName(member.name) %}
 
             {% if member.type.category != "object" and member.optional %}
+                //* Non-constant length optional members use length=0 to denote they aren't present.
+                //* Otherwise we could have length=N and has_member=false, causing reads from an
+                //* uninitialized pointer.
+                {{ assert(member.length == "constant") }}
                 bool has_{{memberName}} = transfer->has_{{memberName}};
                 record->{{memberName}} = nullptr;
                 if (has_{{memberName}})