From 0e7fafebe1bf95a32385dbfcc8a43374b0e3a8fa Mon Sep 17 00:00:00 2001 From: wraitii Date: Sun, 28 Mar 2021 16:48:25 +0000 Subject: [PATCH] Refuse to serialize NaN values. NaN values could not be serialised safely because of the multiple possible NaN numbers. Since NaN values are usually the result of bugs or dangerous code, it seems simpler to refuse to serialise them. (D3205 was a safe-serialization alternative, should the need arise). Fixes #1879 Differential Revision: https://code.wildfiregames.com/D3729 This was SVN commit r25151. --- .../serialization/BinarySerializer.cpp | 24 +++++++++++++++---- source/simulation2/tests/test_Serializer.h | 5 +++- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/source/simulation2/serialization/BinarySerializer.cpp b/source/simulation2/serialization/BinarySerializer.cpp index 96c88abf42..5a78f0ae69 100644 --- a/source/simulation2/serialization/BinarySerializer.cpp +++ b/source/simulation2/serialization/BinarySerializer.cpp @@ -1,4 +1,4 @@ -/* Copyright (C) 2020 Wildfire Games. +/* Copyright (C) 2021 Wildfire Games. * This file is part of 0 A.D. * * 0 A.D. is free software: you can redistribute it and/or modify @@ -257,12 +257,21 @@ void CBinarySerializerScriptImpl::HandleScriptVal(JS::HandleValue val) } else if (protokey == JSProto_Number) { - // Standard Number object - m_Serializer.NumberU8_Unbounded("type", SCRIPT_TYPE_OBJECT_NUMBER); // Get primitive value double d; if (!JS::ToNumber(rq.cx, val, &d)) throw PSERROR_Serialize_ScriptError("JS::ToNumber failed"); + + // Refuse to serialize NaN values: their representation can differ, leading to OOS + // and in general this is indicative of an underlying bug rather than desirable behaviour. + if (std::isnan(d)) + { + LOGERROR("Cannot serialize NaN values."); + throw PSERROR_Serialize_InvalidScriptValue(); + } + + // Standard Number object + m_Serializer.NumberU8_Unbounded("type", SCRIPT_TYPE_OBJECT_NUMBER); m_Serializer.NumberDouble_Unbounded("value", d); break; } @@ -373,12 +382,19 @@ void CBinarySerializerScriptImpl::HandleScriptVal(JS::HandleValue val) } case JSTYPE_NUMBER: { + // Refuse to serialize NaN values: their representation can differ, leading to OOS + // and in general this is indicative of an underlying bug rather than desirable behaviour. + if (val == JS::NaNValue()) + { + LOGERROR("Cannot serialize NaN values."); + throw PSERROR_Serialize_InvalidScriptValue(); + } + // To reduce the size of the serialized data, we handle integers and doubles separately. // We can't check for val.isInt32 and val.isDouble directly, because integer numbers are not guaranteed // to be represented as integers. A number like 33 could be stored as integer on the computer of one player // and as double on the other player's computer. That would cause out of sync errors in multiplayer games because // their binary representation and thus the hash would be different. - double d; d = val.toNumber(); i32 integer; diff --git a/source/simulation2/tests/test_Serializer.h b/source/simulation2/tests/test_Serializer.h index 895074cd1a..c2c3652098 100644 --- a/source/simulation2/tests/test_Serializer.h +++ b/source/simulation2/tests/test_Serializer.h @@ -770,7 +770,10 @@ public: void test_script_nonfinite() { - helper_script_roundtrip("nonfinite", "[0, Infinity, -Infinity, NaN, -1/Infinity]", "[0, Infinity, -Infinity, NaN, -0]"); + helper_script_roundtrip("nonfinite", "[0, Infinity, -Infinity, -1/Infinity]", "[0, Infinity, -Infinity, -0]"); + + TestLogger logger; + TS_ASSERT_THROWS(helper_script_roundtrip("nan", "[NaN]", "[NaN]"), const PSERROR_Serialize_InvalidScriptValue&); } void test_script_property_order()