LF: Add check of nesting in SValue.toValue (#10370)

The conversion of SValue to Value already ensures the resulting value has a serializable type. Here we add a check to ensure it does not overpass the maximum allow nesting.
digital-asset · Jul 22, 2021 · 4a33c03 · 4a33c03
1 parent 91529ee
commit 4a33c03
Show file tree

Hide file tree

Showing 9 changed files with 106 additions and 56 deletions.
diff --git a/compiler/scenario-service/client/src/DA/Daml/LF/PrettyScenario.hs b/compiler/scenario-service/client/src/DA/Daml/LF/PrettyScenario.hs
@@ -410,6 +410,8 @@ prettyScenarioErrorError (Just err) =  do
           scenarioError_ContractIdInContractKeyKey
     ScenarioErrorErrorComparableValueError _ ->
       pure "Attend to compare incomparable values"
+    ScenarioErrorErrorValueExceedsMaxNesting _ ->
+          pure "Value exceeds maximum nesting value of 100"
 
 
 partyDifference :: V.Vector Party -> V.Vector Party -> Doc SyntaxClass

diff --git a/compiler/scenario-service/protos/scenario_service.proto b/compiler/scenario-service/protos/scenario_service.proto
@@ -235,7 +235,7 @@ message ScenarioError {
   repeated TraceMessage trace_log = 8;
 
   // Warnings generated by engine.
-  repeated WarningMessage warnings = 31;
+  repeated WarningMessage warnings = 32;
 
   // The ledger time at the time of the error.
   sfixed64 ledger_time = 9;
@@ -278,6 +278,7 @@ message ScenarioError {
     WronglyTypedContract wronglyTypedContract = 28;
     Empty ComparableValueError = 29;
     ContractIdInContractKey contract_id_in_contract_key = 30;
+    Empty ValueExceedsMaxNesting = 31;
   }
 }
 

diff --git a/...cenario-service/server/src/main/scala/com/digitalasset/daml/lf/scenario/Conversions.scala b/...cenario-service/server/src/main/scala/com/digitalasset/daml/lf/scenario/Conversions.scala
@@ -180,6 +180,8 @@ final class Conversions(
                 builder.setCrash(s"Contract Id freshness Error")
               case NonComparableValues =>
                 builder.setComparableValueError(proto.Empty.newBuilder)
+              case ValueExceedsMaxNesting =>
+                builder.setValueExceedsMaxNesting(proto.Empty.newBuilder)
             }
         }
       case Error.ContractNotEffective(coid, tid, effectiveAt) =>

diff --git a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Pretty.scala b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Pretty.scala
@@ -117,6 +117,8 @@ private[lf] object Pretty {
         text(
           s"Contract IDs are not supported in contract keys: ${key.ensureNoCid.left.toOption.get}"
         )
+      case ValueExceedsMaxNesting =>
+        text(s"Value exceeds maximum nesting value of 100")
     }
   }
 

diff --git a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SValue.scala b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SValue.scala
@@ -24,10 +24,49 @@ import scala.util.hashing.MurmurHash3
   */
 sealed trait SValue {
 
-  import SValue._
+  import SValue.{SValue => _, _}
 
-  def toValue: V[V.ContractId] =
+  def toValue: V[V.ContractId] = SValue.toValue(this)
+
+  def mapContractId(f: V.ContractId => V.ContractId): SValue =
     this match {
+      case SContractId(coid) => SContractId(f(coid))
+      case SEnum(_, _, _) | _: SPrimLit | SToken | STNat(_) | STypeRep(_) => this
+      case SPAP(prim, args, arity) =>
+        val prim2 = prim match {
+          case PClosure(label, expr, vars) =>
+            PClosure(label, expr, vars.map(_.mapContractId(f)))
+          case other => other
+        }
+        val args2 = mapArrayList(args, _.mapContractId(f))
+        SPAP(prim2, args2, arity)
+      case SRecord(tycon, fields, values) =>
+        SRecord(tycon, fields, mapArrayList(values, v => v.mapContractId(f)))
+      case SStruct(fields, values) =>
+        SStruct(fields, mapArrayList(values, v => v.mapContractId(f)))
+      case SVariant(tycon, variant, rank, value) =>
+        SVariant(tycon, variant, rank, value.mapContractId(f))
+      case SList(lst) =>
+        SList(lst.map(_.mapContractId(f)))
+      case SOptional(mbV) =>
+        SOptional(mbV.map(_.mapContractId(f)))
+      case SMap(isTextMap, value) =>
+        SMap(
+          isTextMap,
+          value.iterator.map { case (k, v) => k.mapContractId(f) -> v.mapContractId(f) },
+        )
+      case SAny(ty, value) =>
+        SAny(ty, value.mapContractId(f))
+    }
+}
+
+object SValue {
+
+  def toValue(v: SValue, maxNesting: Int = V.MAXIMUM_NESTING): V[V.ContractId] = {
+    if (maxNesting < 0)
+      throw SError.SErrorDamlException(interpretation.Error.ValueExceedsMaxNesting)
+    val nextMaxNesting = maxNesting - 1
+    v match {
       case SInt64(x) => V.ValueInt64(x)
       case SNumeric(x) => V.ValueNumeric(x)
       case SText(x) => V.ValueText(x)
@@ -43,28 +82,32 @@ sealed trait SValue {
           ImmArray(
             fields.toSeq
               .zip(svalues.asScala)
-              .map({ case (fld, sv) => (Some(fld), sv.toValue) })
+              .map({ case (fld, sv) => (Some(fld), toValue(sv, nextMaxNesting)) })
           ),
         )
       case SVariant(id, variant, _, sv) =>
-        V.ValueVariant(Some(id), variant, sv.toValue)
+        V.ValueVariant(Some(id), variant, toValue(sv, nextMaxNesting))
       case SEnum(id, constructor, _) =>
         V.ValueEnum(Some(id), constructor)
       case SList(lst) =>
-        V.ValueList(lst.map(_.toValue))
+        V.ValueList(lst.map(toValue(_, nextMaxNesting)))
       case SOptional(mbV) =>
-        V.ValueOptional(mbV.map(_.toValue))
+        V.ValueOptional(mbV.map(toValue(_, nextMaxNesting)))
       case SMap(true, entries) =>
         V.ValueTextMap(SortedLookupList(entries.map {
-          case (SText(t), v) => t -> v.toValue
+          case (SText(t), v) => t -> toValue(v, nextMaxNesting)
           case (_, _) =>
             throw SError.SErrorCrash(
               NameOf.qualifiedNameOfCurrentFunc,
               "SValue.toValue: TextMap with non text key",
             )
         }))
       case SMap(false, entries) =>
-        V.ValueGenMap(entries.view.map { case (k, v) => k.toValue -> v.toValue }.to(ImmArray))
+        V.ValueGenMap(
+          entries.view
+            .map { case (k, v) => toValue(k, nextMaxNesting) -> toValue(v, nextMaxNesting) }
+            .to(ImmArray)
+        )
       case SContractId(coid) =>
         V.ValueContractId(coid)
       case _: SStruct | _: SAny | _: SBigNumeric | _: STypeRep | _: STNat | _: SPAP | SToken =>
@@ -73,40 +116,7 @@ sealed trait SValue {
           s"SValue.toValue: unexpected ${getClass.getSimpleName}",
         )
     }
-
-  def mapContractId(f: V.ContractId => V.ContractId): SValue =
-    this match {
-      case SContractId(coid) => SContractId(f(coid))
-      case SEnum(_, _, _) | _: SPrimLit | SToken | STNat(_) | STypeRep(_) => this
-      case SPAP(prim, args, arity) =>
-        val prim2 = prim match {
-          case PClosure(label, expr, vars) =>
-            PClosure(label, expr, vars.map(_.mapContractId(f)))
-          case other => other
-        }
-        val args2 = mapArrayList(args, _.mapContractId(f))
-        SPAP(prim2, args2, arity)
-      case SRecord(tycon, fields, values) =>
-        SRecord(tycon, fields, mapArrayList(values, v => v.mapContractId(f)))
-      case SStruct(fields, values) =>
-        SStruct(fields, mapArrayList(values, v => v.mapContractId(f)))
-      case SVariant(tycon, variant, rank, value) =>
-        SVariant(tycon, variant, rank, value.mapContractId(f))
-      case SList(lst) =>
-        SList(lst.map(_.mapContractId(f)))
-      case SOptional(mbV) =>
-        SOptional(mbV.map(_.mapContractId(f)))
-      case SMap(isTextMap, value) =>
-        SMap(
-          isTextMap,
-          value.iterator.map { case (k, v) => k.mapContractId(f) -> v.mapContractId(f) },
-        )
-      case SAny(ty, value) =>
-        SAny(ty, value.mapContractId(f))
-    }
-}
-
-object SValue {
+  }
 
   /** "Primitives" that can be applied. */
   sealed trait Prim

diff --git a/daml-lf/interpreter/src/test/scala/com/digitalasset/daml/lf/speedy/SValueTest.scala b/daml-lf/interpreter/src/test/scala/com/digitalasset/daml/lf/speedy/SValueTest.scala
@@ -0,0 +1,42 @@
+// Copyright (c) 2021 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package com.daml.lf
+package speedy
+
+import com.daml.lf.data.Ref
+import com.daml.lf.value.Value
+import org.scalatest.matchers.should.Matchers
+import org.scalatest.wordspec.AnyWordSpec
+
+import scala.annotation.tailrec
+import scala.util.{Failure, Success, Try}
+
+class SValueTest extends AnyWordSpec with Matchers {
+
+  "SValue#toValue" should {
+
+    val Nat = Ref.Identifier.assertFromString("-pkgId:Mod:Nat")
+    val Z = Ref.Name.assertFromString("Z")
+    val S = Ref.Name.assertFromString("S")
+
+    @tailrec
+    def toNat(i: Int, acc: SValue = SValue.SVariant(Nat, Z, 0, SValue.SUnit)): SValue =
+      if (i <= 0) acc
+      else toNat(i - 1, SValue.SVariant(Nat, S, 1, acc))
+
+    "rejects excessive nesting" in {
+      // Because Z has a nesting of 1, toNat(Value.MAXIMUM_NESTING) has a nesting of
+      // Value.MAXIMUM_NESTING + 1
+      Try(toNat(Value.MAXIMUM_NESTING).toValue) shouldBe
+        Failure(SError.SErrorDamlException(interpretation.Error.ValueExceedsMaxNesting))
+    }
+
+    "accepts just right nesting" in {
+      // Because Z has a nesting of 1, toNat(Value.MAXIMUM_NESTING - 1) has a nesting of
+      // Value.MAXIMUM_NESTING
+      Try(toNat(Value.MAXIMUM_NESTING - 1)) shouldBe a[Success[_]]
+    }
+  }
+
+}
diff --git a/daml-lf/transaction/src/main/scala/com/daml/lf/Error.scala b/daml-lf/transaction/src/main/scala/com/daml/lf/Error.scala
@@ -104,4 +104,6 @@ object Error {
 
   final case class ContractIdInContractKey(key: Value[ContractId]) extends Error
 
+  final case object ValueExceedsMaxNesting extends Error
+
 }
diff --git a/ledger/sandbox-classic/BUILD.bazel b/ledger/sandbox-classic/BUILD.bazel
@@ -440,13 +440,5 @@ server_conformance_test(
     lf_versions = ["stable"],
     server_args = ["--contract-id-seeding=testing-weak"],
     servers = SERVERS,
-    test_tool_args = [
-        "--include=" + ",".join([
-            "DeeplyNestedValueIT:Accept",
-            "DeeplyNestedValueIT:RejectCreateCommand",
-            "DeeplyNestedValueIT:RejectExerciseCommand",
-            "DeeplyNestedValueIT:RejectCreateArgumentInCreateAndExerciseCommand",
-            "DeeplyNestedValueIT:RejectChoiceArgumentInCreateAndExerciseCommand",
-        ]),
-    ],
+    test_tool_args = ["--include=DeeplyNestedValueIT"],
 )
diff --git a/ledger/sandbox/BUILD.bazel b/ledger/sandbox/BUILD.bazel
@@ -325,10 +325,7 @@ server_conformance_test(
             "DeeplyNestedValueIT:AcceptFetchByKey46",
             "DeeplyNestedValueIT:AcceptFailingLookupByKey46",
             "DeeplyNestedValueIT:AcceptSuccessfulLookupByKey46",
-            "DeeplyNestedValueIT:RejectCreateCommand",
-            "DeeplyNestedValueIT:RejectExerciseCommand",
-            "DeeplyNestedValueIT:RejectCreateArgumentInCreateAndExerciseCommand",
-            "DeeplyNestedValueIT:RejectChoiceArgumentInCreateAndExerciseCommand",
+            "DeeplyNestedValueIT:Reject",
         ]),
     ],
 )
-Original file line number
+Diff line change
@@ Expand Up / @@ -117,6 +117,8 @@ private[lf] object Pretty { @@
             text(
               s"Contract IDs are not supported in contract keys: ${key.ensureNoCid.left.toOption.get}"
             )
+          case ValueExceedsMaxNesting =>
+            text(s"Value exceeds maximum nesting value of 100")
         }
       }
@@ Expand Down @@
Original file line number	Diff line number	Diff line change
Expand Up		@@ -104,4 +104,6 @@ object Error {

		final case class ContractIdInContractKey(key: Value[ContractId]) extends Error

		final case object ValueExceedsMaxNesting extends Error

		}