Binary data converted?

[theboredfish]

Hey

Not sure whether this is a bug or a hole in knowledge...

I have a PHP function invoked from V8Js which returns raw binary data. The idea being that a user writing JS scripts in their sandbox can use this data with another V8Js-invoked PHP function to send it to an API.

The issue is the file appears to undergo a bit of conversion and i'm not sure of the best one-size-fits-all approach that'll work whether the file is binary or not. Ideally, no conversion at all would be best :)

We'll call the function called from JS PHP.get_file() for simplicity and assume that it does a basic file_get_contents() in PHP, returned directly.

• Original file is a 32404 byte PDF
• When examining the length of data to return, in PHP, it gives me the correct value of 32404
• When examining the length of the data in the JS variable i assign the return value to, it gives me 31115
• Using PHP-side functions called from JS to encode/decode base64 is pointless, as problem is back as soon as I decode and return the value.
• if i return a base64 encoded string from my PHP function instead, take the value output from JS and decode it manually, it's also fine. 32404.

The only thing that worked in this case after randomly trying stuff was, instead of returning the value directly, doing a quick conversion first:

function get_file(string $filename): string
{
   .... stuff ....
   $content = file_get_contents($filename);
   return mb_convert_encoding($content, 'UTF-8', 'ASCII');
}

which seems a little odd, given that PHP tells me that the result of the mb_convert_encoding is 46013 but the variable in JS it's directly assigned to now reports it as 32404....the correct number. mb_detect_encoding at this stage gives me nothing, so i hard-coded "ASCII" for testing.

Until i pass the binary data back into another PHP function from my JS. Now it's back to 46013 and a quick mb_convert_encoding the other way (ASCII from UTF-8) gives me back my original 32404 file. mb_detect_encoding before that gives me UTF-8

Encodings give me headaches on good days, so apologies if that was a long-winded way to explain a simple thing. I'm hoping that there is a way this can be resolved WITHOUT the JS code writer having to change their stuff.

Is this a bug? Or something that's easily solvable? Hoping that there's:

• a setting/flag somewhere to prevent conversion taking place at all
• a solution that works regardless of whether the file is binary or not
• maybe even a decent simple base64 decode function for JS. i tried a couple, but all changed the file in some manner or other - not great when you're also using checksums :-p

[chrisbckr]

V8JS_ZSTR uses v8::String::NewFromUtf8, so I think that is the problem.
And explains why using mb_convert_encoding from ASCII to UTF-8 works.
I think that we need to find a way to detect the value encoding and use NewFromOneByte instead.
Maybe we can use mb_detect_encoding from PHP? But this approach will require mbstring module to be active.

[chrisbckr]

Hmmm found something interesting...
zend_string.h => ZSTR_IS_VALID_UTF8()
I'll test this!

[redbullmarky]

@chrisbckr This one is an old one but still a thorn in my side 😂 It's possibly a little down to my lack of knowledge around JS, too. Sounds like there's something in what you've found!! Very promising.

I'm just not sure whether it's my approach that's wrong, or whether there is something already suited to doing this.

[chrisbckr]

Need more tests (and improvements), but...
v8js_convert.cc -> zval_to_v8js

		case IS_STRING:
			value_str = Z_STR_P(value);
			if (ZSTR_LEN(value_str) > std::numeric_limits<int>::max()) {
				zend_throw_exception(php_ce_v8js_exception,
					"String exceeds maximum string length", 0);
				break;
			}

			zval fname, retval;
			zval params[2];
			ZVAL_STRING(&fname, "mb_check_encoding");
			ZVAL_COPY_VALUE(&params[0], value);
			ZVAL_STRING(&params[1], "UTF-8");

			if ((SUCCESS == call_user_function(CG(function_table), NULL, &fname,
									&retval, 2, params)) && (Z_TYPE(retval) == IS_TRUE)) {
				jsValue = V8JS_ZSTR(value_str);
			}
			else {
				jsValue = v8::String::NewFromOneByte(isolate, (unsigned char *)ZSTR_VAL(value_str), v8::NewStringType::kNormal, ZSTR_LEN(value_str)).ToLocalChecked();
			}
			
			break;

And at the end of Makefile.frag

# add mbstring extension, if available
ifneq (,$(realpath $(EXTENSION_DIR)/mbstring.so))
PHP_TEST_SHARED_EXTENSIONS+=-d extension=$(EXTENSION_DIR)/mbstring.so
endif

@stesie do you know any side effect of using NewFromOneByte instead of NewFromUtf8 in this case?

[chrisbckr]

And maybe we can enable this checking with a flag or something like this.

[chrisbckr]

And maybe we can enable this checking with a flag or something like this.

[redbullmarky]

And maybe we can enable this checking with a flag or something like this.

It feels like this is probably the safest option if the PR is a go, but I’ll be quite happy to give this one a good test! (The OP is me from my other account)

[redbullmarky]

i've been playing around with this test script, along with a PNG

<?php

class Foo extends \V8Js
{
	private static $file = __DIR__ . '/test.png';
	
	public function read(): string
	{
		return file_get_contents(self::$file);
	}
	
	public function write(string $content)
	{
		$oldContent = file_get_contents(self::$file);
		echo ($oldContent === $content) ? "Files are the same!" : "Files are different :(";
		echo "\n" . strlen($oldContent) . ' vs ' . strlen($content) . "\n"; // 6339 vs 9362 :(
		echo mb_detect_encoding($content) . "\n"; // utf-8 :(
	}
}

$v = new Foo();
$result = $v->executeString('
	const content = PHP.read();
	print(typeof content + "\n"); // outputs: string
        print(content.length + "\n"); // same as what the actual filesize is :)
	PHP.write(content);
');

Things work great on read() !!

Just struggling with the changes needed around v8js_to_zval to be able to send the thing back. Encoding is detected as UTF-8, so I guess the same preservation treatment for these kind of things needs doing on the way BACK to PHP?

[chrisbckr]

Good point. I will investigate this.

[chrisbckr]

Test this @redbullmarky !
https://github.com/chrisbckr/v8js/tree/php8-binary_data

[redbullmarky]

@chrisbckr ahh man, turns out what I was trying to do wasn’t far off ! Sure, will have a play in a little bit when I get a little time!

[redbullmarky]

Do you know if it’s recursive? e.g. if I have a PHP array and perhaps one string is regular (like a filename) and the other is binary (like the content), does it iterate into the array calling these conversions?

Edit: Had a quick play, and it certainly does handle strings inside of arrays etc, too!

My crude test (I used force array flag to keep it as an array going both ways, else it's a V8Object):

<?php

class Foo extends \V8Js
{
	private static $file = __DIR__ . '/test.png';
	
	public function read(): string
	{
		echo "File: " . self::$file . "\n";
		return file_get_contents(self::$file);
	}
	
	public function readArray(): array
	{
		return [
			'file' => self::$file,
			'content' => $this->read()
		];
	}
	
	public function write(string $content)
	{
		$oldContent = file_get_contents(self::$file);
		echo ($oldContent === $content) ? "Files are the same!" : "Files are different :(";
		echo "\n" . strlen($oldContent) . ' vs ' . strlen($content) . "\n";
		echo mb_detect_encoding($content) . "\n"; // utf-8 :(
	}
	
	public function writeArray($array)
	{
		$this->write($array['content']);
		echo (strlen($array['file']) === strlen(self::$file) ? "YES!" : "NO") . "\n";
	}
	
	public function outLen(string $str)
	{
		echo "str is " . strlen($str) . "\n";
	}
}

$v = new Foo();
$result = $v->executeString('
	const content = PHP.read();
	PHP.write(content);
	
	const arr = PHP.readArray();
	PHP.writeArray(arr);
	
	PHP.outLen(arr.file);
	PHP.outLen(arr.content);
', null, V8Js::FLAG_FORCE_ARRAY);

[chrisbckr]

Just added a flag V8Js::FLAG_CHECK_BINARY_STRING to enable this feature(?) hehe

[redbullmarky]

Awesome! One thing i picked up on testing (as mentioned above, in having to use the FORCE_ARRAY flag - and probably another topic/issue but more of a passing thought for now hehe) - is it possible that when an array (associative or otherwise) is passed FROM php to JS, it can also maintain its type when passed back? e.g. if i do this without force array flag:

class Foo extends V8Js
{
   public function getArray()
   {
      return ['foo' => 'bar'];
   }

  public function setArray(array $array)
  {
  }
}
$v = new Foo();
$v->executeString('
   const arr = PHP.getArray();
   PHP.setArray(arr); 
');

Then you get:

PHP Fatal error:  Uncaught TypeError: Foo::setArray(): Argument #1 ($array) must be of type array, V8Object given. 

I get why returning regular objects is useful, and it probably opens up a can of worms as to WHEN you might want a V8Object or an array (like, what if you constructed an "associative array" in JS, and passed it - how would you know whether to cast it to an array for PHP, or keep it as a V8Object? etc)