Remove runtime inference of unrecognized SFields

src/ripple/protocol/SField.h

@@ -24,8 +24,6 @@
 #include <ripple/json/json_value.h>
 #include <cstdint>
 #include <map>
-#include <memory>
-#include <mutex>
 #include <utility>
 
 namespace ripple {
@@ -104,17 +102,8 @@ field_code(int id, int index)
 /** Identifies fields.
 
     Fields are necessary to tag data in signed transactions so that
-    the binary format of the transaction can be canonicalized.
+    the binary format of the transaction can be canonicalized.  All
-
+    SFields are created at compile time.
-    There are two categories of these fields:
-
-    1.  Those that are created at compile time.
-    2.  Those that are created at run time.
-
-    Both are always const.  Category 1 can only be created in FieldNames.cpp.
-    This is enforced at compile time.  Category 2 can only be created by
-    calling getField with an as yet unused fieldType and fieldValue (or the
-    equivalent fieldCode).
 
     Each SField, once constructed, lives until program termination, and there
     is only one instance per fieldType/fieldValue pair which serves the entire
@@ -156,21 +145,14 @@ public:
     SField& operator=(SField&&) = delete;
 
 public:
-    struct private_access_tag_t;         // public, but still an implementation detail
+    struct private_access_tag_t;   // public, but still an implementation detail
 
     // These constructors can only be called from SField.cpp
     SField (private_access_tag_t, SerializedTypeID tid, int fv,
         const char* fn, int meta = sMD_Default,
         IsSigning signing = IsSigning::yes);
     explicit SField (private_access_tag_t, int fc);
-    SField (private_access_tag_t, SerializedTypeID tid, int fv);
 
-protected:
-    // These constructors can only be called from SField.cpp
-    explicit SField (SerializedTypeID tid, int fv);
-
-public:
-    // getField will dynamically construct a new SField if necessary
     static const SField& getField (int fieldCode);
     static const SField& getField (std::string const& fieldName);
     static const SField& getField (int type, int value)
@@ -270,10 +252,7 @@ public:
 
 private:
     static int num;
-
-    static std::mutex SField_mutex;
     static std::map<int, SField const*> knownCodeToField;
-    static std::map<int, std::unique_ptr<SField const>> unknownCodeToField;
 };
 
 /** A field with a type known at compile time. */

src/ripple/protocol/SOTemplate.h

@@ -20,6 +20,7 @@
 #ifndef RIPPLE_PROTOCOL_SOTEMPLATE_H_INCLUDED
 #define RIPPLE_PROTOCOL_SOTEMPLATE_H_INCLUDED
 
+#include <ripple/basics/contract.h>
 #include <ripple/protocol/SField.h>
 #include <boost/range.hpp>
 #include <memory>
@@ -49,6 +50,8 @@ public:
         : e_field (fieldName)
         , flags (flags)
     {
+        if (! e_field.isUseful())
+            Throw<std::runtime_error> ("SField in SOElement must be useful.");
     }
 };
 

src/ripple/protocol/impl/SField.cpp

@@ -17,7 +17,6 @@
 */
 //==============================================================================
 
-#include <ripple/basics/safe_cast.h>
 #include <ripple/protocol/SField.h>
 #include <cassert>
 #include <string>
@@ -28,11 +27,7 @@ namespace ripple {
 // Storage for static const members.
 SField::IsSigning const SField::notSigning;
 int SField::num = 0;
-std::mutex SField::SField_mutex;
 std::map<int, SField const*> SField::knownCodeToField;
-std::map<int, std::unique_ptr<SField const>> SField::unknownCodeToField;
-
-using StaticScopedLockType = std::lock_guard <std::mutex>;
 
 // Give only this translation unit permission to construct SFields
 struct SField::private_access_tag_t
@@ -40,7 +35,7 @@ struct SField::private_access_tag_t
     explicit private_access_tag_t() = default;
 };
 
-SField::private_access_tag_t access;
+static SField::private_access_tag_t access;
 
 // Construct all compile-time SFields, and register them in the knownCodeToField
 // database:
@@ -270,29 +265,6 @@ SField::SField(private_access_tag_t, int fc)
     knownCodeToField[fieldCode] = this;
 }
 
-// call with the map mutex to protect num.
-// This is naturally done with no extra expense
-// from getField(int code).
-SField::SField(SerializedTypeID tid, int fv)
-    : fieldCode (field_code (tid, fv))
-    , fieldType (tid)
-    , fieldValue (fv)
-    , fieldName (std::to_string (tid) + '/' + std::to_string (fv))
-    , fieldMeta (sMD_Default)
-    , fieldNum (++num)
-    , signingField (IsSigning::yes)
-    , jsonName (fieldName.c_str())
-{
-    assert ((fv != 1) || ((tid != STI_ARRAY) && (tid != STI_OBJECT)));
-}
-
-SField::SField(private_access_tag_t,
-    SerializedTypeID tid, int fv)
-    : SField(tid, fv)
-{
-    knownCodeToField[fieldCode] = this;
-}
-
 SField const&
 SField::getField (int code)
 {
@@ -300,54 +272,9 @@ SField::getField (int code)
 
     if (it != knownCodeToField.end ())
     {
-        // 99+% of the time, it will be a valid, known field
         return * (it->second);
     }
-
+    return sfInvalid;
-    int type = code >> 16;
-    int field = code & 0xffff;
-
-    // Don't dynamically extend types that have no binary encoding.
-    if ((field > 255) || (code < 0))
-        return sfInvalid;
-
-    switch (type)
-    {
-    // Types we are willing to dynamically extend
-    // types (common)
-    case STI_UINT16:
-    case STI_UINT32:
-    case STI_UINT64:
-    case STI_HASH128:
-    case STI_HASH256:
-    case STI_AMOUNT:
-    case STI_VL:
-    case STI_ACCOUNT:
-    case STI_OBJECT:
-    case STI_ARRAY:
-    // types (uncommon)
-    case STI_UINT8:
-    case STI_HASH160:
-    case STI_PATHSET:
-    case STI_VECTOR256:
-        break;
-
-    default:
-        return sfInvalid;
-    }
-
-    {
-        // Lookup in the run-time data base, and create if it does not
-        // yet exist.
-        StaticScopedLockType sl (SField_mutex);
-
-        auto it = unknownCodeToField.find (code);
-
-        if (it != unknownCodeToField.end ())
-            return * (it->second);
-        return *(unknownCodeToField[code] = std::unique_ptr<SField const>(
-                       new SField(safe_cast<SerializedTypeID>(type), field)));
-    }
 }
 
 int SField::compare (SField const& f1, SField const& f2)
@@ -373,15 +300,6 @@ SField::getField (std::string const& fieldName)
         if (fieldPair.second->fieldName == fieldName)
             return * (fieldPair.second);
     }
-    {
-        StaticScopedLockType sl (SField_mutex);
-
-        for (auto const & fieldPair : unknownCodeToField)
-        {
-            if (fieldPair.second->fieldName == fieldName)
-                return * (fieldPair.second);
-        }
-    }
     return sfInvalid;
 }
 

src/ripple/protocol/impl/SOTemplate.cpp

@@ -28,18 +28,20 @@ void SOTemplate::push_back (SOElement const& r)
     //
     if (mIndex.empty ())
     {
-        // Unmapped indices will be set to -1
+        // Unmapped indices are initialized to -1
         //
         mIndex.resize (SField::getNumFields () + 1, -1);
     }
 
     // Make sure the field's index is in range
     //
-    assert (r.e_field.getNum () < mIndex.size ());
+    if (r.e_field.getNum() <= 0 || r.e_field.getNum() >= mIndex.size())
+        Throw<std::runtime_error> ("Invalid field index for SOTemplate.");
 
     // Make sure that this field hasn't already been assigned
     //
-    assert (getIndex (r.e_field) == -1);
+    if (getIndex (r.e_field) != -1)
+        Throw<std::runtime_error> ("Duplicate field index for SOTemplate.");
 
     // Add the field to the index mapping table
     //

src/test/protocol/STObject_test.cpp

@@ -235,12 +235,54 @@ public:
         testcase ("serialization");
 
         unexpected (sfGeneric.isUseful (), "sfGeneric must not be useful");
+        {
+            // Try to put sfGeneric in an SOTemplate.
+            SOTemplate elements;
+            except<std::runtime_error>( [&]()
+                {
+                    elements.push_back (SOElement (sfGeneric, SOE_REQUIRED));
+                });
+        }
+
+        unexpected (sfInvalid.isUseful (), "sfInvalid must not be useful");
+        {
+            // Test return of sfInvalid.
+            auto testInvalid = [this] (SerializedTypeID tid, int fv)
+            {
+                SField const& shouldBeInvalid {SField::getField (tid, fv)};
+                BEAST_EXPECT (shouldBeInvalid == sfInvalid);
+            };
+            testInvalid (STI_VL, 255);
+            testInvalid (STI_HASH256, 255);
+            testInvalid (STI_UINT32, 255);
+            testInvalid (STI_VECTOR256, 255);
+            testInvalid (STI_OBJECT, 255);
+        }
+        {
+            // Try to put sfInvalid in an SOTemplate.
+            SOTemplate elements;
+            except<std::runtime_error>( [&]()
+                {
+                    elements.push_back (SOElement (sfInvalid, SOE_REQUIRED));
+                });
+        }
+        {
+            // Try to put the same SField into an SOTemplate twice.
+            SOTemplate elements;
+            elements.push_back (SOElement (sfAccount, SOE_REQUIRED));
+            except<std::runtime_error>( [&]()
+                {
+                    elements.push_back (SOElement (sfAccount, SOE_REQUIRED));
+                });
+        }
+
+        // Put a variety of SFields of different types in an SOTemplate.
+        SField const& sfTestVL = sfMasterSignature;
+        SField const& sfTestH256 = sfCheckID;
+        SField const& sfTestU32 = sfSettleDelay;
+        SField const& sfTestV256 = sfAmendments;
+        SField const& sfTestObject = sfMajority;
 
-        SField const& sfTestVL = SField::getField (STI_VL, 255);
-        SField const& sfTestH256 = SField::getField (STI_HASH256, 255);
-        SField const& sfTestU32 = SField::getField (STI_UINT32, 255);
-        SField const& sfTestV256 = SField::getField(STI_VECTOR256, 255);
-        SField const& sfTestObject = SField::getField (STI_OBJECT, 255);
 
         SOTemplate elements;
         elements.push_back (SOElement (sfFlags, SOE_REQUIRED));
@@ -639,15 +681,15 @@ public:
 
         try
         {
-            std::array<std::uint8_t, 5> const payload {{ 0xee, 0xee, 0xe1, 0xee, 0xee }};
+            std::array<std::uint8_t, 7> const payload {
+                { 0xe9, 0x12, 0xab, 0xcd, 0x12, 0xfe, 0xdc }};
             SerialIter sit{makeSlice(payload)};
             auto obj = std::make_shared<STArray>(sit, sfMetadata);
             BEAST_EXPECT(!obj);
         }
         catch (std::exception const& e)
         {
-            BEAST_EXPECT(strcmp(e.what(),
+            BEAST_EXPECT(strcmp(e.what(), "Duplicate field detected") == 0);
-                "Duplicate field detected") == 0);
         }
 
         try
@@ -659,45 +701,7 @@ public:
         }
         catch (std::exception const& e)
         {
-            BEAST_EXPECT(strcmp(e.what(),
+            BEAST_EXPECT(strcmp(e.what(), "Duplicate field detected") == 0);
-                "Duplicate field detected") == 0);
-        }
-
-        try
-        {
-            std::array<std::uint8_t, 250> const payload
-            {{
-                0x12, 0x00, 0x65, 0x24, 0x00, 0x00, 0x00, 0x00, 0x20, 0x1e, 0x00, 0x4f,
-                0x00, 0x00, 0x20, 0x1f, 0x03, 0xf6, 0x00, 0x00, 0x20, 0x20, 0x00, 0x00,
-                0x00, 0x00, 0x35, 0x00, 0x59, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x68,
-                0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x73, 0x00, 0x81, 0x14,
-                0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x65, 0x24, 0x00, 0x00,
-                0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe5, 0xfe, 0xf3, 0xe7, 0xe5, 0x65,
-                0x24, 0x00, 0x00, 0x00, 0x00, 0x20, 0x1e, 0x00, 0x6f, 0x00, 0x00, 0x20,
-                0x1f, 0x03, 0xf6, 0x00, 0x00, 0x20, 0x20, 0x00, 0x00, 0x00, 0x00, 0x35,
-                0x00, 0x59, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x68, 0x40, 0x00, 0x00,
-                0x00, 0x00, 0x00, 0x02, 0x00, 0x12, 0x00, 0x65, 0x24, 0x00, 0x00, 0x00,
-                0x00, 0x20, 0x1e, 0x00, 0x4f, 0x00, 0x00, 0x20, 0x1f, 0x03, 0xf6, 0x00,
-                0x00, 0x20, 0x20, 0x00, 0x00, 0x00, 0x00, 0x35, 0x24, 0x59, 0x00, 0x00,
-                0x00, 0x00, 0x00, 0x00, 0x68, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02,
-                0x00, 0x54, 0x72, 0x61, 0x6e, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00,
-                0x00, 0x65, 0x24, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe5,
-                0xfe, 0xf3, 0xe7, 0xe5, 0x65, 0x24, 0x00, 0x00, 0x00, 0x00, 0x20, 0x1e,
-                0x00, 0x6f, 0x00, 0x00, 0x20, 0xf6, 0x00, 0x00, 0x03, 0x1f, 0x20, 0x20,
-                0x00, 0x00, 0x00, 0x00, 0x35, 0x00, 0x59, 0x00, 0x00, 0x00, 0x00, 0x00,
-                0x00, 0x68, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x73, 0x00,
-                0x81, 0x14, 0x00, 0x10, 0x00, 0x73, 0x00, 0x81, 0x14, 0x00, 0x10, 0x00,
-                0x00, 0x00, 0x00, 0x26, 0x00, 0x00, 0x00, 0x00, 0xe5, 0xfe
-            }};
-
-            SerialIter sit{makeSlice(payload)};
-            auto obj = std::make_shared<STTx>(sit);
-            BEAST_EXPECT(!obj);
-        }
-        catch (std::exception const& e)
-        {
-            BEAST_EXPECT(strcmp(e.what(),
-                "Duplicate field detected") == 0);
         }
     }
 

src/test/protocol/STTx_test.cpp

@@ -28,6 +28,8 @@
 #include <ripple/basics/Slice.h>
 #include <ripple/protocol/messages.h>
 
+#include <memory>
+
 namespace ripple {
 
 class STTx_test : public beast::unit_test::suite
@@ -1218,7 +1220,7 @@ public:
             0x12, 0x12, 0x12, 0xff
         };
 
-        constexpr unsigned char dupField[] =
+        constexpr unsigned char payload3[] =
         {
             0x12, 0x00, 0x65, 0x24, 0x00, 0x00, 0x00, 0x00, 0x20, 0x1e, 0x00, 0x4f,
             0x00, 0x00, 0x20, 0x1f, 0x03, 0xf6, 0x00, 0x00, 0x20, 0x20, 0x00, 0x00,
@@ -1243,6 +1245,173 @@ public:
             0x00, 0x00, 0x00, 0x26, 0x00, 0x00, 0x00, 0x00, 0xe5, 0xfe
         };
 
+        // Construct an STObject with 11 levels of object nesting so the
+        // maximum nesting level exception is thrown.
+        {
+            // Construct an SOTemplate to get the ball rolling on building
+            // an STObject that can contain another STObject.
+            SOTemplate recurse;
+            recurse.push_back (SOElement {sfTransactionMetaData, SOE_OPTIONAL});
+            recurse.push_back (SOElement {sfTransactionHash, SOE_OPTIONAL});
+
+            // Make an STObject that nests objects ten levels deep.  There's
+            // a minimum transaction size we must meet, so include a hash256.
+            uint256 const hash {42u};
+            auto inner =
+                std::make_unique<STObject> (recurse, sfTransactionMetaData);
+            inner->setFieldH256 (sfTransactionHash, hash);
+
+            for (int i = 1; i < 10; ++i)
+            {
+                auto outer =
+                    std::make_unique<STObject> (recurse, sfTransactionMetaData);
+                outer->set (std::move (inner));
+                outer->setFieldH256 (sfTransactionHash, hash);
+                inner = std::move (outer);
+            }
+            {
+                // A ten-deep nested STObject should throw an exception that
+                // there is no sfTransactionType field.
+                Serializer const tenDeep {inner->getSerializer()};
+                SerialIter tenSit {tenDeep.slice()};
+                try
+                {
+                    auto stx = std::make_shared<ripple::STTx const>(tenSit);
+                    fail ("STTx construction should have thrown.");
+                }
+                catch (std::runtime_error const& ex)
+                {
+                    BEAST_EXPECT (
+                        std::strcmp (ex.what(), "Field not found") == 0);
+                }
+            }
+
+            // Add one more level of nesting and we should get an error
+            // about excessive nesting.
+            STObject outer {recurse, sfTransactionMetaData};
+            outer.set (std::move (inner));
+            outer.setFieldH256 (sfTransactionHash, hash);
+
+            Serializer const tooDeep {outer.getSerializer()};
+            SerialIter tooDeepSit {tooDeep.slice()};
+            try
+            {
+                auto stx = std::make_shared<ripple::STTx const>(tooDeepSit);
+                fail ("STTx construction should have thrown.");
+            }
+            catch (std::runtime_error const& ex)
+            {
+                BEAST_EXPECT (std::strcmp (ex.what(),
+                    "Maximum nesting depth of STVar exceeded") == 0);
+            }
+        }
+        {
+            // Construct an STObject with 11 levels of nesting so the
+            // maximum nesting level exception is thrown.  This time
+            // we're nesting arrays in addition to objects.
+
+            // Construct an SOTemplate to get the ball rolling on building
+            // an STObject that can contain an STArray.
+            SOTemplate recurse;
+            recurse.push_back (SOElement {sfTransactionMetaData, SOE_OPTIONAL});
+            recurse.push_back (SOElement {sfTransactionHash, SOE_OPTIONAL});
+            recurse.push_back (SOElement {sfTemplate, SOE_OPTIONAL});
+
+            // Make an STObject that nests ten levels deep alternating objects
+            // and arrays.  Include a hash256 to meet the minimum transaction
+            // size.
+            uint256 const hash {42u};
+            STObject inner = {recurse, sfTransactionMetaData};
+            inner.setFieldH256 (sfTransactionHash, hash);
+
+            for (int i = 1; i < 5; ++i)
+            {
+                STObject outer {recurse, sfTransactionMetaData};
+                outer.setFieldH256 (sfTransactionHash, hash);
+                STArray& array {outer.peekFieldArray (sfTemplate)};
+                array.push_back (std::move (inner));
+                inner = std::move (outer);
+            }
+            {
+                // A nine-deep nested STObject/STArray should throw an
+                // exception that there is no sfTransactionType field.
+                Serializer const nineDeep {inner.getSerializer()};
+                SerialIter nineSit {nineDeep.slice()};
+                try
+                {
+                    auto stx = std::make_shared<ripple::STTx const>(nineSit);
+                    fail ("STTx construction should have thrown.");
+                }
+                catch (std::runtime_error const& ex)
+                {
+                    BEAST_EXPECT (
+                        std::strcmp (ex.what(), "Field not found") == 0);
+                }
+            }
+
+            // Add one more level of nesting and we should get an error
+            // about excessive nesting.
+            STObject outer {recurse, sfTransactionMetaData};
+            outer.setFieldH256 (sfTransactionHash, hash);
+            STArray& array {outer.peekFieldArray (sfTemplate)};
+            array.push_back (std::move (inner));
+
+            Serializer const tooDeep {outer.getSerializer()};
+            SerialIter tooDeepSit {tooDeep.slice()};
+            try
+            {
+                auto stx = std::make_shared<ripple::STTx const>(tooDeepSit);
+                fail ("STTx construction should have thrown.");
+            }
+            catch (std::runtime_error const& ex)
+            {
+                BEAST_EXPECT (std::strcmp (ex.what(),
+                    "Maximum nesting depth of STVar exceeded") == 0);
+            }
+        }
+
+        {
+            // Make an otherwise legit STTx with a duplicate field.  Should
+            // generate an exception when we deserialize.
+            auto const keypair = randomKeyPair (KeyType::secp256k1);
+            STTx acctSet (ttACCOUNT_SET,
+                [&keypair](auto& obj)
+                {
+                    obj.setAccountID (sfAccount, calcAccountID(keypair.first));
+                    obj.setFieldU32 (sfSequence, 7);
+                    obj.setFieldAmount (sfFee, STAmount (2557891634ull));
+                    obj.setFieldVL (sfSigningPubKey, keypair.first.slice());
+                    obj.setFieldU32 (sfSetFlag, 0x0DDBA11);
+                    obj.setFieldU32 (sfClearFlag, 0xB01DFACE);
+                });
+
+            Serializer serialized {acctSet.getSerializer()};
+            {
+                // Verify we have a valid transaction.
+                SerialIter sit {serialized.slice()};
+                auto stx = std::make_shared<ripple::STTx const>(sit);
+            }
+
+            // Tweak the serialized data to change the ClearFlag to
+            // a SetFlag.  This will leave us with two SetFlag fields
+            // which we should trap as a duplicate field.
+            BEAST_EXPECT (serialized.modData()[15] == sfClearFlag.fieldValue);
+            serialized.modData()[15] = sfSetFlag.fieldValue;
+
+            SerialIter sit {serialized.slice()};
+            try
+            {
+                auto stx = std::make_shared<ripple::STTx const>(sit);
+                fail("An exception should have been thrown");
+            }
+            catch (std::exception const& ex)
+            {
+                BEAST_EXPECT(
+                    strcmp(ex.what(), "Duplicate field detected") == 0);
+            }
+        }
+
+        // Exercise the three raw transactions.
         try
         {
             protocol::TMTransaction tx2;
@@ -1253,10 +1422,9 @@ public:
             auto stx = std::make_shared<ripple::STTx const>(sit);
             fail("An exception should have been thrown");
         }
-        catch (std::exception const& e)
+        catch (std::exception const& ex)
         {
-            BEAST_EXPECT(strcmp(e.what(),
+            BEAST_EXPECT(strcmp(ex.what(), "Unknown field") == 0);
-                "Maximum nesting depth of STVar exceeded") == 0);
         }
 
         try
@@ -1265,22 +1433,20 @@ public:
             auto stx = std::make_shared<ripple::STTx const>(sit);
             fail("An exception should have been thrown");
         }
-        catch (std::exception const& e)
+        catch (std::exception const& ex)
         {
-            BEAST_EXPECT(strcmp(e.what(),
+            BEAST_EXPECT(strcmp(ex.what(), "Unknown field") == 0);
-                "Maximum nesting depth of STVar exceeded") == 0);
         }
 
         try
         {
-            ripple::SerialIter sit (Slice{dupField, sizeof(dupField)});
+            ripple::SerialIter sit {payload3};
             auto stx = std::make_shared<ripple::STTx const>(sit);
             fail("An exception should have been thrown");
         }
-        catch (std::exception const& e)
+        catch (std::exception const& ex)
         {
-            BEAST_EXPECT(strcmp(e.what(),
+            BEAST_EXPECT(strcmp(ex.what(), "Unknown field") == 0);
-                "Duplicate field detected") == 0);
         }
     }