Skip to content

Commit 2491ba9

Browse files
copybara-githubcopybara-github
committed
Merge pull request #2050 from sahvx655-wq:map-key-double-cast-ub
PiperOrigin-RevId: 948509158
2 parents 63cba4b + 84e7773 commit 2491ba9

2 files changed

Lines changed: 107 additions & 26 deletions

File tree

‎common/values/parsed_map_field_value.cc‎

Lines changed: 33 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include <cstdint>
1919
#include <limits>
2020
#include <memory>
21+
#include <optional>
2122
#include <string>
2223
#include <utility>
2324

@@ -27,12 +28,12 @@
2728
#include "absl/log/absl_check.h"
2829
#include "absl/status/status.h"
2930
#include "absl/status/statusor.h"
30-
#include "absl/types/optional.h"
3131
#include "common/value.h"
3232
#include "common/values/values.h"
3333
#include "extensions/protobuf/internal/map_reflection.h"
3434
#include "internal/json.h"
3535
#include "internal/message_equality.h"
36+
#include "internal/number.h"
3637
#include "internal/status_macros.h"
3738
#include "internal/well_known_types.h"
3839
#include "google/protobuf/arena.h"
@@ -187,7 +188,7 @@ size_t ParsedMapFieldValue::Size() const {
187188

188189
namespace {
189190

190-
absl::optional<int32_t> ValueAsInt32(const Value& value) {
191+
std::optional<int32_t> ValueAsInt32(const Value& value) {
191192
if (auto int_value = value.AsInt();
192193
int_value &&
193194
int_value->NativeValue() >= std::numeric_limits<int32_t>::min() &&
@@ -197,32 +198,36 @@ absl::optional<int32_t> ValueAsInt32(const Value& value) {
197198
uint_value &&
198199
uint_value->NativeValue() <= std::numeric_limits<int32_t>::max()) {
199200
return static_cast<int32_t>(uint_value->NativeValue());
200-
} else if (auto double_value = value.AsDouble();
201-
double_value &&
202-
static_cast<double>(static_cast<int32_t>(
203-
double_value->NativeValue())) == double_value->NativeValue()) {
204-
return static_cast<int32_t>(double_value->NativeValue());
201+
} else if (auto double_value = value.AsDouble(); double_value) {
202+
auto number = internal::Number::FromDouble(double_value->NativeValue());
203+
if (number.LosslessConvertibleToInt()) {
204+
int64_t wide_value = number.AsInt();
205+
if (wide_value >= std::numeric_limits<int32_t>::min() &&
206+
wide_value <= std::numeric_limits<int32_t>::max()) {
207+
return static_cast<int32_t>(wide_value);
208+
}
209+
}
205210
}
206211
return std::nullopt;
207212
}
208213

209-
absl::optional<int64_t> ValueAsInt64(const Value& value) {
214+
std::optional<int64_t> ValueAsInt64(const Value& value) {
210215
if (auto int_value = value.AsInt(); int_value) {
211216
return int_value->NativeValue();
212217
} else if (auto uint_value = value.AsUint();
213218
uint_value &&
214219
uint_value->NativeValue() <= std::numeric_limits<int64_t>::max()) {
215220
return static_cast<int64_t>(uint_value->NativeValue());
216-
} else if (auto double_value = value.AsDouble();
217-
double_value &&
218-
static_cast<double>(static_cast<int64_t>(
219-
double_value->NativeValue())) == double_value->NativeValue()) {
220-
return static_cast<int64_t>(double_value->NativeValue());
221+
} else if (auto double_value = value.AsDouble(); double_value) {
222+
auto number = internal::Number::FromDouble(double_value->NativeValue());
223+
if (number.LosslessConvertibleToInt()) {
224+
return number.AsInt();
225+
}
221226
}
222227
return std::nullopt;
223228
}
224229

225-
absl::optional<uint32_t> ValueAsUInt32(const Value& value) {
230+
std::optional<uint32_t> ValueAsUInt32(const Value& value) {
226231
if (auto int_value = value.AsInt();
227232
int_value && int_value->NativeValue() >= 0 &&
228233
int_value->NativeValue() <= std::numeric_limits<uint32_t>::max()) {
@@ -231,26 +236,29 @@ absl::optional<uint32_t> ValueAsUInt32(const Value& value) {
231236
uint_value && uint_value->NativeValue() <=
232237
std::numeric_limits<uint32_t>::max()) {
233238
return static_cast<uint32_t>(uint_value->NativeValue());
234-
} else if (auto double_value = value.AsDouble();
235-
double_value &&
236-
static_cast<double>(static_cast<uint32_t>(
237-
double_value->NativeValue())) == double_value->NativeValue()) {
238-
return static_cast<uint32_t>(double_value->NativeValue());
239+
} else if (auto double_value = value.AsDouble(); double_value) {
240+
auto number = internal::Number::FromDouble(double_value->NativeValue());
241+
if (number.LosslessConvertibleToUint()) {
242+
uint64_t wide_value = number.AsUint();
243+
if (wide_value <= std::numeric_limits<uint32_t>::max()) {
244+
return static_cast<uint32_t>(wide_value);
245+
}
246+
}
239247
}
240248
return std::nullopt;
241249
}
242250

243-
absl::optional<uint64_t> ValueAsUInt64(const Value& value) {
251+
std::optional<uint64_t> ValueAsUInt64(const Value& value) {
244252
if (auto int_value = value.AsInt();
245253
int_value && int_value->NativeValue() >= 0) {
246254
return static_cast<uint64_t>(int_value->NativeValue());
247255
} else if (auto uint_value = value.AsUint(); uint_value) {
248256
return uint_value->NativeValue();
249-
} else if (auto double_value = value.AsDouble();
250-
double_value &&
251-
static_cast<double>(static_cast<uint64_t>(
252-
double_value->NativeValue())) == double_value->NativeValue()) {
253-
return static_cast<uint64_t>(double_value->NativeValue());
257+
} else if (auto double_value = value.AsDouble(); double_value) {
258+
auto number = internal::Number::FromDouble(double_value->NativeValue());
259+
if (number.LosslessConvertibleToUint()) {
260+
return number.AsUint();
261+
}
254262
}
255263
return std::nullopt;
256264
}

‎common/values/parsed_map_field_value_test.cc‎

Lines changed: 74 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15+
#include <limits>
16+
#include <optional>
1517
#include <utility>
1618
#include <vector>
1719

@@ -21,7 +23,6 @@
2123
#include "absl/status/statusor.h"
2224
#include "absl/strings/string_view.h"
2325
#include "absl/time/time.h"
24-
#include "absl/types/optional.h"
2526
#include "common/memory.h"
2627
#include "common/type.h"
2728
#include "common/value.h"
@@ -225,6 +226,78 @@ TEST_F(ParsedMapFieldValueTest, Find) {
225226
IsOkAndHolds(Eq(std::nullopt)));
226227
}
227228

229+
TEST_F(ParsedMapFieldValueTest, FindDoubleKeyConversion) {
230+
// Lookups on integer-keyed maps coerce double keys following CEL's numeric
231+
// semantics ({1: 'x'}[1.0] resolves to key 1). A double that is out of range,
232+
// negative for an unsigned key, or non-finite must be reported as absent
233+
// rather than narrowed to the key type: converting such a double straight to
234+
// an integer with static_cast is undefined behaviour.
235+
ParsedMapFieldValue int64_map(
236+
DynamicParseTextProto<TestAllTypesProto3>(R"pb(
237+
map_int64_int64 { key: 1 value: 100 }
238+
)pb"),
239+
DynamicGetField<TestAllTypesProto3>("map_int64_int64"), arena());
240+
EXPECT_THAT(int64_map.Find(DoubleValue(1.0), descriptor_pool(),
241+
message_factory(), arena()),
242+
IsOkAndHolds(Optional(IntValueIs(100))));
243+
EXPECT_THAT(int64_map.Find(DoubleValue(1e19), descriptor_pool(),
244+
message_factory(), arena()),
245+
IsOkAndHolds(Eq(std::nullopt)));
246+
EXPECT_THAT(int64_map.Find(DoubleValue(-1e19), descriptor_pool(),
247+
message_factory(), arena()),
248+
IsOkAndHolds(Eq(std::nullopt)));
249+
EXPECT_THAT(
250+
int64_map.Find(DoubleValue(std::numeric_limits<double>::infinity()),
251+
descriptor_pool(), message_factory(), arena()),
252+
IsOkAndHolds(Eq(std::nullopt)));
253+
EXPECT_THAT(
254+
int64_map.Find(DoubleValue(std::numeric_limits<double>::quiet_NaN()),
255+
descriptor_pool(), message_factory(), arena()),
256+
IsOkAndHolds(Eq(std::nullopt)));
257+
258+
ParsedMapFieldValue uint64_map(
259+
DynamicParseTextProto<TestAllTypesProto3>(R"pb(
260+
map_uint64_uint64 { key: 1 value: 100 }
261+
)pb"),
262+
DynamicGetField<TestAllTypesProto3>("map_uint64_uint64"), arena());
263+
EXPECT_THAT(uint64_map.Find(DoubleValue(1.0), descriptor_pool(),
264+
message_factory(), arena()),
265+
IsOkAndHolds(Optional(UintValueIs(100))));
266+
EXPECT_THAT(uint64_map.Find(DoubleValue(-1.0), descriptor_pool(),
267+
message_factory(), arena()),
268+
IsOkAndHolds(Eq(std::nullopt)));
269+
EXPECT_THAT(uint64_map.Find(DoubleValue(1e20), descriptor_pool(),
270+
message_factory(), arena()),
271+
IsOkAndHolds(Eq(std::nullopt)));
272+
273+
ParsedMapFieldValue int32_map(
274+
DynamicParseTextProto<TestAllTypesProto3>(R"pb(
275+
map_int32_int32 { key: 1 value: 100 }
276+
)pb"),
277+
DynamicGetField<TestAllTypesProto3>("map_int32_int32"), arena());
278+
EXPECT_THAT(int32_map.Find(DoubleValue(1.0), descriptor_pool(),
279+
message_factory(), arena()),
280+
IsOkAndHolds(Optional(IntValueIs(100))));
281+
EXPECT_THAT(int32_map.Find(DoubleValue(1e19), descriptor_pool(),
282+
message_factory(), arena()),
283+
IsOkAndHolds(Eq(std::nullopt)));
284+
285+
ParsedMapFieldValue uint32_map(
286+
DynamicParseTextProto<TestAllTypesProto3>(R"pb(
287+
map_uint32_uint32 { key: 1 value: 100 }
288+
)pb"),
289+
DynamicGetField<TestAllTypesProto3>("map_uint32_uint32"), arena());
290+
EXPECT_THAT(uint32_map.Find(DoubleValue(1.0), descriptor_pool(),
291+
message_factory(), arena()),
292+
IsOkAndHolds(Optional(UintValueIs(100))));
293+
EXPECT_THAT(uint32_map.Find(DoubleValue(-1.0), descriptor_pool(),
294+
message_factory(), arena()),
295+
IsOkAndHolds(Eq(std::nullopt)));
296+
EXPECT_THAT(uint32_map.Find(DoubleValue(1e19), descriptor_pool(),
297+
message_factory(), arena()),
298+
IsOkAndHolds(Eq(std::nullopt)));
299+
}
300+
228301
TEST_F(ParsedMapFieldValueTest, Has) {
229302
ParsedMapFieldValue value(
230303
DynamicParseTextProto<TestAllTypesProto3>(R"pb(

0 commit comments

Comments
 (0)