Breaking Change: Validate UTF-8 in string setters, [as previously ann…

…ounced](https://protobuf.dev/news/2023-12-27/#php-breaking-changes). Pure-PHP was already validating UTF-8, but this makes the C extension validate also. PiperOrigin-RevId: 597695655
protocolbuffers · Jan 12, 2024 · d14dbbc · d14dbbc
1 parent d1444e2
commit d14dbbc
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 15 deletions.
diff --git a/php/ext/google/protobuf/convert.c b/php/ext/google/protobuf/convert.c
@@ -370,21 +370,23 @@ bool Convert_PhpToUpb(zval* php_val, upb_MessageValue* upb_val, TypeInfo type,
       return to_bool(php_val, &upb_val->bool_val);
     case kUpb_CType_String:
     case kUpb_CType_Bytes: {
-      char* ptr;
-      size_t size;
-
       if (!to_string(php_val)) return false;
 
-      size = Z_STRLEN_P(php_val);
+      char* ptr = Z_STRVAL_P(php_val);
+      size_t size = Z_STRLEN_P(php_val);
+
+      if (type.type == kUpb_CType_String && !utf8_range_IsValid(ptr, size)) {
+        zend_throw_exception_ex(NULL, 0, "Invalid UTF-8 in string data");
+        return false;
+      }
 
       // If arena is NULL we reference the input zval.
       // The resulting upb_StringView will only be value while the zval is
       // alive.
       if (arena) {
-        ptr = upb_Arena_Malloc(arena, size);
-        memcpy(ptr, Z_STRVAL_P(php_val), size);
-      } else {
-        ptr = Z_STRVAL_P(php_val);
+        char* copy = upb_Arena_Malloc(arena, size);
+        memcpy(copy, ptr, size);
+        ptr = copy;
       }
 
       upb_val->str_val = upb_StringView_FromDataAndSize(ptr, size);

diff --git a/php/tests/GeneratedClassTest.php b/php/tests/GeneratedClassTest.php
@@ -449,6 +449,19 @@ public function testStringField()
         $this->assertSame('1', $m->getOptionalString());
     }
 
+    #########################################################
+    # Test invalid UTF-8
+    #########################################################
+
+    public function testInvalidUtf8StringFails()
+    {
+        $m = new TestMessage();
+
+        // Invalid UTF-8 is rejected.
+        $this->expectException(Exception::class);
+        $m->setOptionalString("\xff");
+    }
+
     #########################################################
     # Test bytes field.
     #########################################################
@@ -474,13 +487,12 @@ public function testBytesField()
         $this->assertSame('1', $m->getOptionalBytes());
     }
 
-      public function testBytesFieldInvalidUTF8Success()
-      {
-          $m = new TestMessage();
-          $hex = hex2bin("ff");
-          $m->setOptionalBytes($hex);
-          $this->assertTrue(true);
-      }
+    public function testBytesFieldInvalidUTF8Success()
+    {
+        $m = new TestMessage();
+        $m->setOptionalBytes("\xff");
+        $this->assertSame("\xff", $m->getOptionalBytes());
+    }
 
     #########################################################
     # Test message field.