[v5.0.1] Add threshold for maxTreeDepth for serializers #798

jozanek · 2022-05-09T18:50:15Z

Partially closes #759, #781

…lizers.

codecov · 2022-05-09T19:34:09Z

Codecov Report

Merging #798 (91853dd) into v5.0.1-RC (743123c) will decrease coverage by 0.09%.
The diff coverage is 87.50%.

❗ Current head 91853dd differs from pull request most recent head 595ef21. Consider uploading reports for the commit 595ef21 to get more accurate results

@@              Coverage Diff              @@
##           v5.0.1-RC     #798      +/-   ##
=============================================
- Coverage      71.89%   71.79%   -0.10%     
=============================================
  Files            248      248              
  Lines          18942    18829     -113     
  Branches         634      573      -61     
=============================================
- Hits           13618    13519      -99     
+ Misses          5324     5310      -14

Impacted Files	Coverage Δ
...ala/sigmastate/serialization/ValueSerializer.scala	`68.69% <65.21%> (+0.41%)`	⬆️
...cala/sigmastate/serialization/DataSerializer.scala	`98.75% <97.36%> (+1.34%)`	⬆️
sigmastate/src/main/scala/sigmastate/Values.scala	`82.89% <100.00%> (+0.11%)`	⬆️
...cala/sigmastate/serialization/TypeSerializer.scala	`99.15% <100.00%> (+0.02%)`	⬆️
.../main/scala/sigmastate/utils/SigmaByteWriter.scala	`45.37% <100.00%> (+2.65%)`	⬆️
.../src/main/scala/scalan/primitives/NumericOps.scala	`73.91% <0.00%> (-4.35%)`	⬇️
core/src/main/scala/scalan/DefRewriting.scala	`72.94% <0.00%> (-1.18%)`	⬇️
...ain/scala/special/collection/CollsOverArrays.scala	`86.74% <0.00%> (-0.47%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 743123c...595ef21. Read the comment docs.

aslesarenko · 2022-05-11T10:28:40Z

sigmastate/src/test/scala/sigmastate/serialization/DeserializationResilience.scala

@@ -255,6 +288,12 @@ class DeserializationResilience extends SerializationSpecification
      ValueSerializer.deserialize(reader(ValueSerializer.serialize(expr), maxTreeDepth = 3))


Please make sure the thrown exception comes from TypeSerializer. I.e. that the test is actually testing anything.

Updated in new version.

aslesarenko · 2022-05-11T10:29:18Z

sigmastate/src/test/scala/sigmastate/serialization/DeserializationResilience.scala

+  property("max recursive call depth is checked in writer.level for TypeSerializer") {
+    val expr = Tuple(Tuple(IntConstant(1)))
+    an[SerializeCallDepthExceeded] should be thrownBy
+      ValueSerializer.serialize(expr, writer(maxTreeDepth = 2))


Same issues as with the previous test I think.

Correct, the TypeSerializer was completely missing handling for maxTreeDepth for both de/serializing, added to both method and updated tests.

aslesarenko · 2022-05-17T11:13:14Z

sigmastate/src/main/scala/sigmastate/serialization/TypeSerializer.scala

    }
+    w.level = depth


@jozanek please reintroduce changes so that most of the code remain the same and number of red and green lines is minimal. Otherwise I cannot estimate what was actually changed.

Github diff tool is quite puzzled, had to break formatting to show it properly. I can add it in additional commit after PR is reviewed.

aslesarenko · 2022-05-17T11:15:32Z

sigmastate/src/main/scala/sigmastate/serialization/TypeSerializer.scala

    tpe
  }

  private def getArgType(r: SigmaByteReader, primId: Int, depth: Int) =
    if (primId == 0)
-      deserialize(r, depth + 1)
+      deserialize(r)
    else


Please rollback changes in deserializer, it part of the consensus, where as serializer is not.
This PR don't need to touch consensus code.

…r, update tests.

jozanek · 2023-02-17T22:55:50Z

@aslesarenko can you review again please and possibly merge?

aslesarenko · 2023-02-20T15:51:58Z

@jozanek, this is low prioriity and considering how much unmerged code is there in develop already I'm putting this PR on hold.

aslesarenko · 2023-08-28T10:21:47Z

After giving more thought into this I think this feature will complicate maintenance.
Depth limits on serialisation should exactly match measurements on deserialisation.
Any difference may lead to subtle bugs in dApps using Sigma (Appkit, Fleet).
And adversary can create malicious bytes directly anyway, so the value of this checks is small.

v5.x-control-max-tree-depth: Add threshold for maxTreeDepth for seria…

3cad7f4

…lizers.

jozanek requested a review from aslesarenko May 9, 2022 18:50

aslesarenko added 2 commits May 11, 2022 12:31

v5.x-control-max-tree-depth: fixes during review

a65db82

v5.x-control-max-tree-depth: fixed test

8bee45d

aslesarenko requested changes May 11, 2022

View reviewed changes

aslesarenko requested changes May 18, 2022

View reviewed changes

aslesarenko added this to the v5.x milestone Jun 1, 2022

aslesarenko changed the title ~~Add threshold for maxTreeDepth for serializers~~ [v5.0.1] Add threshold for maxTreeDepth for serializers Jun 1, 2022

aslesarenko mentioned this pull request Jun 1, 2022

Release candidate v5.0.1 (please ignore) #809

Merged

aslesarenko changed the base branch from v5.0-add-jit-costing-for-tests to v5.0.1-RC June 1, 2022 11:28

aslesarenko mentioned this pull request Jun 1, 2022

Release candidate v5.0.3 #811

Merged

jozanek force-pushed the v5.x-control-max-tree-depth branch 3 times, most recently from 129fdb0 to 91853dd Compare June 7, 2022 19:41

v5.x-control-max-tree-depth: Add maxTreeDepth check for TypeSerialize…

595ef21

…r, update tests.

jozanek force-pushed the v5.x-control-max-tree-depth branch from 91853dd to 595ef21 Compare June 7, 2022 19:49

aslesarenko force-pushed the v5.0.1-RC branch from 2668480 to 9705a29 Compare October 13, 2022 13:24

aslesarenko marked this pull request as draft December 23, 2022 11:39

aslesarenko changed the base branch from v5.0.1-RC to v5.0.4-RC January 4, 2023 16:15

aslesarenko changed the base branch from v5.0.4-RC to develop February 20, 2023 15:47

aslesarenko closed this Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v5.0.1] Add threshold for maxTreeDepth for serializers #798

[v5.0.1] Add threshold for maxTreeDepth for serializers #798

jozanek commented May 9, 2022 •

edited by aslesarenko

Loading

codecov bot commented May 9, 2022 •

edited

Loading

aslesarenko May 11, 2022 •

edited

Loading

jozanek May 17, 2022

aslesarenko May 11, 2022

jozanek May 17, 2022

aslesarenko May 17, 2022

jozanek Jun 7, 2022

aslesarenko May 17, 2022

jozanek Jul 27, 2022

jozanek commented Feb 17, 2023

aslesarenko commented Feb 20, 2023

aslesarenko commented Aug 28, 2023

		@@ -255,6 +288,12 @@ class DeserializationResilience extends SerializationSpecification
		ValueSerializer.deserialize(reader(ValueSerializer.serialize(expr), maxTreeDepth = 3))

[v5.0.1] Add threshold for maxTreeDepth for serializers #798

[v5.0.1] Add threshold for maxTreeDepth for serializers #798

Conversation

jozanek commented May 9, 2022 • edited by aslesarenko Loading

codecov bot commented May 9, 2022 • edited Loading

Codecov Report

aslesarenko May 11, 2022 • edited Loading

Choose a reason for hiding this comment

jozanek May 17, 2022

Choose a reason for hiding this comment

aslesarenko May 11, 2022

Choose a reason for hiding this comment

jozanek May 17, 2022

Choose a reason for hiding this comment

aslesarenko May 17, 2022

Choose a reason for hiding this comment

jozanek Jun 7, 2022

Choose a reason for hiding this comment

aslesarenko May 17, 2022

Choose a reason for hiding this comment

jozanek Jul 27, 2022

Choose a reason for hiding this comment

jozanek commented Feb 17, 2023

aslesarenko commented Feb 20, 2023

aslesarenko commented Aug 28, 2023

jozanek commented May 9, 2022 •

edited by aslesarenko

Loading

codecov bot commented May 9, 2022 •

edited

Loading

aslesarenko May 11, 2022 •

edited

Loading