[fidl][hlcpp] Fix bug in flexible union move constructor
This CL fixes a bug in HLCPP's move constructor for flexible unions. It
was only copying the unknown_data_.bytes rather than the entire
unknown_data_, leaving the handles field uninitialized. This manifested
as a misaligned pointer crash in the union's destructor.
Test: fx test fidl-hlcpp-conformance-test
Fixed: 62819
Change-Id: I8c0a213310a8dc99dcf662a9be56b4aed6fb5b0e
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/443495
Commit-Queue: Mitchell Kember <mkember@google.com>
Reviewed-by: Felix Zhu <fcz@google.com>
Testability-Review: Felix Zhu <fcz@google.com>
diff --git a/garnet/go/src/fidl/compiler/backend/goldens/doc_comments.test.json.cc.golden b/garnet/go/src/fidl/compiler/backend/goldens/doc_comments.test.json.cc.golden
index 861bc57..bea952b 100644
--- a/garnet/go/src/fidl/compiler/backend/goldens/doc_comments.test.json.cc.golden
+++ b/garnet/go/src/fidl/compiler/backend/goldens/doc_comments.test.json.cc.golden
@@ -453,8 +453,8 @@
case static_cast<fidl_xunion_tag_t>(Tag::Invalid):
break;
default:
- new (&unknown_data_.bytes) decltype(unknown_data_.bytes);
- unknown_data_.bytes = std::move(other.unknown_data_.bytes);
+ new (&unknown_data_) decltype(unknown_data_);
+ unknown_data_ = std::move(other.unknown_data_);
break;
}
}
diff --git a/garnet/go/src/fidl/compiler/backend/goldens/handles_in_types.test.json.cc.golden b/garnet/go/src/fidl/compiler/backend/goldens/handles_in_types.test.json.cc.golden
index 2ab1026..319d5ef 100644
--- a/garnet/go/src/fidl/compiler/backend/goldens/handles_in_types.test.json.cc.golden
+++ b/garnet/go/src/fidl/compiler/backend/goldens/handles_in_types.test.json.cc.golden
@@ -43,8 +43,8 @@
case static_cast<fidl_xunion_tag_t>(Tag::Invalid):
break;
default:
- new (&unknown_data_.bytes) decltype(unknown_data_.bytes);
- unknown_data_.bytes = std::move(other.unknown_data_.bytes);
+ new (&unknown_data_) decltype(unknown_data_);
+ unknown_data_ = std::move(other.unknown_data_);
break;
}
}
diff --git a/garnet/go/src/fidl/compiler/backend/goldens/placement_of_attributes.test.json.cc.golden b/garnet/go/src/fidl/compiler/backend/goldens/placement_of_attributes.test.json.cc.golden
index 371582e..39acce4 100644
--- a/garnet/go/src/fidl/compiler/backend/goldens/placement_of_attributes.test.json.cc.golden
+++ b/garnet/go/src/fidl/compiler/backend/goldens/placement_of_attributes.test.json.cc.golden
@@ -156,8 +156,8 @@
case static_cast<fidl_xunion_tag_t>(Tag::Invalid):
break;
default:
- new (&unknown_data_.bytes) decltype(unknown_data_.bytes);
- unknown_data_.bytes = std::move(other.unknown_data_.bytes);
+ new (&unknown_data_) decltype(unknown_data_);
+ unknown_data_ = std::move(other.unknown_data_);
break;
}
}
diff --git a/garnet/go/src/fidl/compiler/backend/goldens/request_flexible_envelope.test.json.cc.golden b/garnet/go/src/fidl/compiler/backend/goldens/request_flexible_envelope.test.json.cc.golden
index 917de76..7158dcf 100644
--- a/garnet/go/src/fidl/compiler/backend/goldens/request_flexible_envelope.test.json.cc.golden
+++ b/garnet/go/src/fidl/compiler/backend/goldens/request_flexible_envelope.test.json.cc.golden
@@ -223,8 +223,8 @@
case static_cast<fidl_xunion_tag_t>(Tag::Invalid):
break;
default:
- new (&unknown_data_.bytes) decltype(unknown_data_.bytes);
- unknown_data_.bytes = std::move(other.unknown_data_.bytes);
+ new (&unknown_data_) decltype(unknown_data_);
+ unknown_data_ = std::move(other.unknown_data_);
break;
}
}
diff --git a/garnet/go/src/fidl/compiler/backend/goldens/union.test.json.cc.golden b/garnet/go/src/fidl/compiler/backend/goldens/union.test.json.cc.golden
index a1456fb..f5bd192 100644
--- a/garnet/go/src/fidl/compiler/backend/goldens/union.test.json.cc.golden
+++ b/garnet/go/src/fidl/compiler/backend/goldens/union.test.json.cc.golden
@@ -1646,8 +1646,8 @@
case static_cast<fidl_xunion_tag_t>(Tag::Invalid):
break;
default:
- new (&unknown_data_.bytes) decltype(unknown_data_.bytes);
- unknown_data_.bytes = std::move(other.unknown_data_.bytes);
+ new (&unknown_data_) decltype(unknown_data_);
+ unknown_data_ = std::move(other.unknown_data_);
break;
}
}
@@ -2043,8 +2043,8 @@
case static_cast<fidl_xunion_tag_t>(Tag::Invalid):
break;
default:
- new (&unknown_data_.bytes) decltype(unknown_data_.bytes);
- unknown_data_.bytes = std::move(other.unknown_data_.bytes);
+ new (&unknown_data_) decltype(unknown_data_);
+ unknown_data_ = std::move(other.unknown_data_);
break;
}
}
@@ -2564,8 +2564,8 @@
case static_cast<fidl_xunion_tag_t>(Tag::Invalid):
break;
default:
- new (&unknown_data_.bytes) decltype(unknown_data_.bytes);
- unknown_data_.bytes = std::move(other.unknown_data_.bytes);
+ new (&unknown_data_) decltype(unknown_data_);
+ unknown_data_ = std::move(other.unknown_data_);
break;
}
}
@@ -2806,8 +2806,8 @@
case static_cast<fidl_xunion_tag_t>(Tag::Invalid):
break;
default:
- new (&unknown_data_.bytes) decltype(unknown_data_.bytes);
- unknown_data_.bytes = std::move(other.unknown_data_.bytes);
+ new (&unknown_data_) decltype(unknown_data_);
+ unknown_data_ = std::move(other.unknown_data_);
break;
}
}
@@ -3046,8 +3046,8 @@
case static_cast<fidl_xunion_tag_t>(Tag::Invalid):
break;
default:
- new (&unknown_data_.bytes) decltype(unknown_data_.bytes);
- unknown_data_.bytes = std::move(other.unknown_data_.bytes);
+ new (&unknown_data_) decltype(unknown_data_);
+ unknown_data_ = std::move(other.unknown_data_);
break;
}
}
@@ -3382,8 +3382,8 @@
case static_cast<fidl_xunion_tag_t>(Tag::Invalid):
break;
default:
- new (&unknown_data_.bytes) decltype(unknown_data_.bytes);
- unknown_data_.bytes = std::move(other.unknown_data_.bytes);
+ new (&unknown_data_) decltype(unknown_data_);
+ unknown_data_ = std::move(other.unknown_data_);
break;
}
}
@@ -3931,8 +3931,8 @@
case static_cast<fidl_xunion_tag_t>(Tag::Invalid):
break;
default:
- new (&unknown_data_.bytes) decltype(unknown_data_.bytes);
- unknown_data_.bytes = std::move(other.unknown_data_.bytes);
+ new (&unknown_data_) decltype(unknown_data_);
+ unknown_data_ = std::move(other.unknown_data_);
break;
}
}
@@ -4153,8 +4153,8 @@
case static_cast<fidl_xunion_tag_t>(Tag::Invalid):
break;
default:
- new (&unknown_data_.bytes) decltype(unknown_data_.bytes);
- unknown_data_.bytes = std::move(other.unknown_data_.bytes);
+ new (&unknown_data_) decltype(unknown_data_);
+ unknown_data_ = std::move(other.unknown_data_);
break;
}
}
diff --git a/src/tests/fidl/conformance_suite/union.gidl b/src/tests/fidl/conformance_suite/union.gidl
index 078bdf0..2448b30 100644
--- a/src/tests/fidl/conformance_suite/union.gidl
+++ b/src/tests/fidl/conformance_suite/union.gidl
@@ -96,8 +96,7 @@
success("FlexibleResourceUnionUnknownValue") {
// llcpp is tested by hand since it cannot construct unknown unions (and
// they do not store bytes & handles), see fxr/423117
- // TODO(fxbug.dev/62819): Fix crash in HLCPP.
- bindings_denylist = [llcpp,hlcpp],
+ bindings_denylist = [llcpp],
value = TestFlexibleResourceXUnionInStruct {
xu: SampleResourceXUnion{
1234: {
@@ -117,8 +116,7 @@
success("FlexibleResourceUnionUnknownValueWithHandles") {
// TODO(fxbug.dev/36441): Implement handles in all backends.
- // TODO(fxbug.dev/62819): Fix crash in HLCPP.
- bindings_denylist = [llcpp,hlcpp],
+ bindings_denylist = [llcpp],
handle_defs = {
#0 = event(),
#1 = event(),
diff --git a/tools/fidl/fidlgen_hlcpp/codegen/union.tmpl.go b/tools/fidl/fidlgen_hlcpp/codegen/union.tmpl.go
index 4f85309..1543433 100644
--- a/tools/fidl/fidlgen_hlcpp/codegen/union.tmpl.go
+++ b/tools/fidl/fidlgen_hlcpp/codegen/union.tmpl.go
@@ -255,8 +255,8 @@
break;
{{- if .IsFlexible }}
default:
- new (&unknown_data_.bytes) decltype(unknown_data_.bytes);
- unknown_data_.bytes = std::move(other.unknown_data_.bytes);
+ new (&unknown_data_) decltype(unknown_data_);
+ unknown_data_= std::move(other.unknown_data_);
break;
{{- end }}
}
