Revert "Fix js conformance tests. (#3604)" (#3633)

This reverts commit 2bd55a9f.

conformance/failure_list_js.txt

+Required.Proto3.ProtobufInput.RepeatedScalarSelectsLast.INT32.ProtobufOutput
+Required.Proto3.ProtobufInput.RepeatedScalarSelectsLast.UINT32.ProtobufOutput
+Required.Proto3.ProtobufInput.ValidDataRepeated.BOOL.ProtobufOutput
+Required.Proto3.ProtobufInput.ValidDataRepeated.DOUBLE.ProtobufOutput
+Required.Proto3.ProtobufInput.ValidDataRepeated.FIXED32.ProtobufOutput
+Required.Proto3.ProtobufInput.ValidDataRepeated.FIXED64.ProtobufOutput
+Required.Proto3.ProtobufInput.ValidDataRepeated.FLOAT.ProtobufOutput
+Required.Proto3.ProtobufInput.ValidDataRepeated.INT32.ProtobufOutput
+Required.Proto3.ProtobufInput.ValidDataRepeated.INT64.ProtobufOutput
+Required.Proto3.ProtobufInput.ValidDataRepeated.SFIXED32.ProtobufOutput
+Required.Proto3.ProtobufInput.ValidDataRepeated.SFIXED64.ProtobufOutput
+Required.Proto3.ProtobufInput.ValidDataRepeated.SINT32.ProtobufOutput
+Required.Proto3.ProtobufInput.ValidDataRepeated.SINT64.ProtobufOutput
+Required.Proto3.ProtobufInput.ValidDataRepeated.UINT32.ProtobufOutput
+Required.Proto3.ProtobufInput.ValidDataRepeated.UINT64.ProtobufOutput
+Required.Proto2.ProtobufInput.RepeatedScalarSelectsLast.INT32.ProtobufOutput
+Required.Proto2.ProtobufInput.RepeatedScalarSelectsLast.UINT32.ProtobufOutput
+Required.Proto2.ProtobufInput.ValidDataRepeated.INT32.ProtobufOutput
+Required.Proto2.ProtobufInput.ValidDataRepeated.UINT32.ProtobufOutput

js/binary/decoder.js

@@ -582,24 +582,27 @@ jspb.BinaryDecoder.prototype.readUnsignedVarint32 = function() {
   x |= (temp & 0x0F) << 28;
   if (temp < 128) {
     // We're reading the high bits of an unsigned varint. The byte we just read
-    // also contains bits 33 through 35, which we're going to discard.
+    // also contains bits 33 through 35, which we're going to discard. Those
+    // bits _must_ be zero, or the encoding is invalid.
+    goog.asserts.assert((temp & 0xF0) == 0);
     this.cursor_ += 5;
     goog.asserts.assert(this.cursor_ <= this.end_);
     return x >>> 0;
   }
 
-  // If we get here, we need to truncate coming bytes. However we need to make
-  // sure cursor place is correct.
-  var i = 5;
-  do {
-    goog.asserts.assert(i < 10);
-    if (bytes[this.cursor_ + i] < 128) {
-      break;
-    }
-    i++;
-  } while (1);
+  // If we get here, we're reading the sign extension of a negative 32-bit int.
+  // We can skip these bytes, as we know in advance that they have to be all
+  // 1's if the varint is correctly encoded. Since we also know the value is
+  // negative, we don't have to coerce it to unsigned before we return it.
+
+  goog.asserts.assert((temp & 0xF0) == 0xF0);
+  goog.asserts.assert(bytes[this.cursor_ + 5] == 0xFF);
+  goog.asserts.assert(bytes[this.cursor_ + 6] == 0xFF);
+  goog.asserts.assert(bytes[this.cursor_ + 7] == 0xFF);
+  goog.asserts.assert(bytes[this.cursor_ + 8] == 0xFF);
+  goog.asserts.assert(bytes[this.cursor_ + 9] == 0x01);
 
-  this.cursor_ += i + 1;
+  this.cursor_ += 10;
   goog.asserts.assert(this.cursor_ <= this.end_);
   return x;
 };

js/binary/decoder_test.js

@@ -270,7 +270,24 @@ describe('binaryDecoderTest', function() {
     assertThrows(function() {decoder.readSignedVarint64()});
     decoder.reset();
     assertThrows(function() {decoder.readZigzagVarint64()});
-    decoder.reset();
+
+    // Positive 32-bit varints encoded with 1 bits in positions 33 through 35
+    // should trigger assertions.
+    decoder.setBlock([255, 255, 255, 255, 0x1F]);
+    assertThrows(function() {decoder.readUnsignedVarint32()});
+
+    decoder.setBlock([255, 255, 255, 255, 0x2F]);
+    assertThrows(function() {decoder.readUnsignedVarint32()});
+
+    decoder.setBlock([255, 255, 255, 255, 0x4F]);
+    assertThrows(function() {decoder.readUnsignedVarint32()});
+
+    // Negative 32-bit varints encoded with non-1 bits in the high dword should
+    // trigger assertions.
+    decoder.setBlock([255, 255, 255, 255, 255, 255, 0, 255, 255, 1]);
+    assertThrows(function() {decoder.readUnsignedVarint32()});
+
+    decoder.setBlock([255, 255, 255, 255, 255, 255, 255, 255, 255, 0]);
     assertThrows(function() {decoder.readUnsignedVarint32()});
   });
 

js/compatibility_tests/v3.0.0/binary/decoder_test.js

@@ -228,7 +228,24 @@ describe('binaryDecoderTest', function() {
     assertThrows(function() {decoder.readSignedVarint64()});
     decoder.reset();
     assertThrows(function() {decoder.readZigzagVarint64()});
-    decoder.reset();
+
+    // Positive 32-bit varints encoded with 1 bits in positions 33 through 35
+    // should trigger assertions.
+    decoder.setBlock([255, 255, 255, 255, 0x1F]);
+    assertThrows(function() {decoder.readUnsignedVarint32()});
+
+    decoder.setBlock([255, 255, 255, 255, 0x2F]);
+    assertThrows(function() {decoder.readUnsignedVarint32()});
+
+    decoder.setBlock([255, 255, 255, 255, 0x4F]);
+    assertThrows(function() {decoder.readUnsignedVarint32()});
+
+    // Negative 32-bit varints encoded with non-1 bits in the high dword should
+    // trigger assertions.
+    decoder.setBlock([255, 255, 255, 255, 255, 255, 0, 255, 255, 1]);
+    assertThrows(function() {decoder.readUnsignedVarint32()});
+
+    decoder.setBlock([255, 255, 255, 255, 255, 255, 255, 255, 255, 0]);
     assertThrows(function() {decoder.readUnsignedVarint32()});
   });
 

js/compatibility_tests/v3.1.0/binary/decoder_test.js

@@ -228,7 +228,24 @@ describe('binaryDecoderTest', function() {
     assertThrows(function() {decoder.readSignedVarint64()});
     decoder.reset();
     assertThrows(function() {decoder.readZigzagVarint64()});
-    decoder.reset();
+
+    // Positive 32-bit varints encoded with 1 bits in positions 33 through 35
+    // should trigger assertions.
+    decoder.setBlock([255, 255, 255, 255, 0x1F]);
+    assertThrows(function() {decoder.readUnsignedVarint32()});
+
+    decoder.setBlock([255, 255, 255, 255, 0x2F]);
+    assertThrows(function() {decoder.readUnsignedVarint32()});
+
+    decoder.setBlock([255, 255, 255, 255, 0x4F]);
+    assertThrows(function() {decoder.readUnsignedVarint32()});
+
+    // Negative 32-bit varints encoded with non-1 bits in the high dword should
+    // trigger assertions.
+    decoder.setBlock([255, 255, 255, 255, 255, 255, 0, 255, 255, 1]);
+    assertThrows(function() {decoder.readUnsignedVarint32()});
+
+    decoder.setBlock([255, 255, 255, 255, 255, 255, 255, 255, 255, 0]);
     assertThrows(function() {decoder.readUnsignedVarint32()});
   });
 

src/google/protobuf/compiler/js/js_generator.cc

@@ -2876,29 +2876,6 @@ void Generator::GenerateClassDeserializeBinaryField(
                         "Group" : "Message",
           "grpfield", (field->type() == FieldDescriptor::TYPE_GROUP) ?
                       (SimpleItoa(field->number()) + ", ") : "");
-    } else if (field->is_repeated() &&
-               field->cpp_type() != FieldDescriptor::CPPTYPE_STRING) {
-      printer->Print(
-          "      if (reader.getWireType() == 2) {\n"
-          "        var value = /** @type {$fieldtype_packed$} */ "
-          "(reader.readPacked$reader$());\n"
-          "        msg.set$list_name$(value);\n"
-          "      } else {\n"
-          "        var value = /** @type {$fieldtype$} */ "
-          "(reader.read$reader$());\n"
-          "        msg.add$name$(value);\n"
-          "      }\n",
-          "fieldtype_packed", JSFieldTypeAnnotation(options, field, false, true,
-                                             /* singular_if_not_packed */ false,
-                                             BYTES_U8),
-          "fieldtype", JSFieldTypeAnnotation(options, field, false, true,
-                                             /* singular_if_not_packed */ true,
-                                             BYTES_U8),
-          "reader", JSBinaryReaderMethodType(field),
-          "list_name", JSGetterName(options, field),
-          "name", JSGetterName(options, field,
-                               BYTES_DEFAULT, /* drop_list = */ true)
-      );
     } else {
       printer->Print(
           "      var value = /** @type {$fieldtype$} */ "
@@ -2910,15 +2887,14 @@ void Generator::GenerateClassDeserializeBinaryField(
           JSBinaryReadWriteMethodName(field, /* is_writer = */ false));
     }
 
-    if (field->is_repeated() &&
-        (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE ||
-         field->cpp_type() == FieldDescriptor::CPPTYPE_STRING)) {
+    if (field->is_repeated() && !field->is_packed()) {
       printer->Print(
           "      msg.add$name$(value);\n", "name",
           JSGetterName(options, field, BYTES_DEFAULT, /* drop_list = */ true));
-    } else if (!field->is_repeated()) {
-      // Singular fields, receive a |value| as the field's value ; set this as
-      // the field's value directly.
+    } else {
+      // Singular fields, and packed repeated fields, receive a |value| either
+      // as the field's value or as the array of all the field's values; set
+      // this as the field's value directly.
       printer->Print(
           "      msg.set$name$(value);\n",
           "name", JSGetterName(options, field));