Sfoglia il codice sorgente

Add modify operation to ExtensionFieldValueSet.

During decoding, modifications are frequently made to the extension
values in ExtensionFieldValueSet. In particular for repeated extensions,
these modifications include modifications to copy-on-write data types.
Because the _modify accessor hasn't been standardised yet,
ExtensionFieldValueSet implements get/set only, forcing a modification
of existing data to be done by a get/set pair. This will always emit a
copy-on-write operation.

The result of the above is that decoding repeated extensions is a
quadratic operation, making it diabolically slow. oss-fuzz caught this
behaviour.

This patch adds a non-API modify function to ExtensionFieldValueSet and
uses it on all decoding paths. This code simply encapsulates a somewhat
delicate function invocation that manages to convince the Swift compiler
to use Dictionary's _modify accessor for the underlying operation. The
result is that the oss-fuzz sample runtime decreases from 140s to 5s on
my machine.

This patch will likely have miscellaneous performance improvements
across the board. It adds the oss-fuzz case for regression purposes as
well.
Cory Benfield 5 anni fa
parent
commit
4f9d97261e

BIN
FuzzTesting/FailCases/clusterfuzz-testcase-minimized-FuzzBinary_release-5649532441460736


+ 8 - 10
Sources/SwiftProtobuf/BinaryDecoder.swift

@@ -1110,18 +1110,16 @@ internal struct BinaryDecoder: Decoder {
     ) throws {
         assert(!consumed)
         assert(fieldNumber == ext.fieldNumber)
-        var fieldValue = values[fieldNumber]
-        // Message/Group extensions both will call back into the matching
-        // decode methods, so the recursion depth will be tracked there.
-        if fieldValue != nil {
-            try fieldValue!.decodeExtensionField(decoder: &self)
-        } else {
-            fieldValue = try ext._protobuf_newField(decoder: &self)
-        }
-        if consumed {
+
+        try values.modify(index: fieldNumber) { fieldValue in
+            // Message/Group extensions both will call back into the matching
+            // decode methods, so the recursion depth will be tracked there.
             if fieldValue != nil {
-                values[fieldNumber] = fieldValue
+                try fieldValue!.decodeExtensionField(decoder: &self)
             } else {
+                fieldValue = try ext._protobuf_newField(decoder: &self)
+            }
+            if consumed && fieldValue == nil {
                 // Really things should never get here, if the decoder says
                 // the bytes were consumed, then there should have been a
                 // field that consumed them (existing or created). This

+ 6 - 0
Sources/SwiftProtobuf/ExtensionFieldValueSet.swift

@@ -78,6 +78,12 @@ public struct ExtensionFieldValueSet: Hashable {
     set { values[index] = newValue }
   }
 
+  mutating func modify<ReturnType>(index: Int, _ modifier: (inout AnyExtensionField?) throws -> ReturnType) rethrows -> ReturnType {
+    // This internal helper exists to invoke the _modify accessor on Dictionary for the given operation, which can avoid CoWs
+    // during the modification operation.
+    return try modifier(&values[index])
+  }
+
   public var isInitialized: Bool {
     for (_, v) in values {
       if !v.isInitialized {

+ 6 - 12
Sources/SwiftProtobuf/JSONDecoder.swift

@@ -724,18 +724,12 @@ internal struct JSONDecoder: Decoder {
     // Force-unwrap: we can only get here if the extension exists.
     let ext = scanner.extensions[messageType, fieldNumber]!
 
-    var fieldValue = values[fieldNumber]
-    if fieldValue != nil {
-      try fieldValue!.decodeExtensionField(decoder: &self)
-    } else {
-      fieldValue = try ext._protobuf_newField(decoder: &self)
-    }
-    // If the value was `null`, then the 'else' clause will return nil, as there
-    // is nothing to assign. If the api ever supports merging JSON into an
-    // object to update it, then the 'then' clause likely should be update to
-    // support clearing the the value rather that keeping its current value.
-    if fieldValue != nil {
-      values[fieldNumber] = fieldValue
+    try values.modify(index: fieldNumber) { fieldValue in
+      if fieldValue != nil {
+        try fieldValue!.decodeExtensionField(decoder: &self)
+      } else {
+        fieldValue = try ext._protobuf_newField(decoder: &self)
+      }
     }
   }
 }

+ 12 - 13
Sources/SwiftProtobuf/TextFormatDecoder.swift

@@ -708,19 +708,18 @@ internal struct TextFormatDecoder: Decoder {
 
     mutating func decodeExtensionField(values: inout ExtensionFieldValueSet, messageType: Message.Type, fieldNumber: Int) throws {
         if let ext = scanner.extensions?[messageType, fieldNumber] {
-            var fieldValue = values[fieldNumber]
-            if fieldValue != nil {
-                try fieldValue!.decodeExtensionField(decoder: &self)
-            } else {
-                fieldValue = try ext._protobuf_newField(decoder: &self)
-            }
-            if fieldValue != nil {
-                values[fieldNumber] = fieldValue
-            } else {
-                // Really things should never get here, for TextFormat, decoding
-                // the value should always work or throw an error.  This specific
-                // error result is to allow this to be more detectable.
-                throw TextFormatDecodingError.internalExtensionError
+            try values.modify(index: fieldNumber) { fieldValue in
+                if fieldValue != nil {
+                    try fieldValue!.decodeExtensionField(decoder: &self)
+                } else {
+                    fieldValue = try ext._protobuf_newField(decoder: &self)
+                }
+                if fieldValue == nil {
+                    // Really things should never get here, for TextFormat, decoding
+                    // the value should always work or throw an error.  This specific
+                    // error result is to allow this to be more detectable.
+                    throw TextFormatDecodingError.internalExtensionError
+                }
             }
         }
     }