Skip to content

Commit

Permalink
* Added V8Js::FLAG_CHECK_BINARY_STRING
Browse files Browse the repository at this point in the history
  • Loading branch information
@chrisbckr
chrisbckr committed Feb 25, 2023
1 parent 91fdb85 commit fd3246b
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 24 deletions.
1 change: 1 addition & 0 deletions php_v8js_macros.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ extern "C" {
#define V8JS_FLAG_NONE (1<<0)
#define V8JS_FLAG_FORCE_ARRAY (1<<1)
#define V8JS_FLAG_PROPAGATE_PHP_EXCEPTIONS (1<<2)
#define V8JS_FLAG_CHECK_BINARY_STRING (1<<3)


/* These are not defined by Zend */
Expand Down
1 change: 1 addition & 0 deletions v8js_class.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1080,6 +1080,7 @@ PHP_MINIT_FUNCTION(v8js_class) /* {{{ */
zend_declare_class_constant_long(php_ce_v8js, ZEND_STRL("FLAG_NONE"), V8JS_FLAG_NONE);
zend_declare_class_constant_long(php_ce_v8js, ZEND_STRL("FLAG_FORCE_ARRAY"), V8JS_FLAG_FORCE_ARRAY);
zend_declare_class_constant_long(php_ce_v8js, ZEND_STRL("FLAG_PROPAGATE_PHP_EXCEPTIONS"), V8JS_FLAG_PROPAGATE_PHP_EXCEPTIONS);
zend_declare_class_constant_long(php_ce_v8js, ZEND_STRL("FLAG_CHECK_BINARY_STRING"), V8JS_FLAG_CHECK_BINARY_STRING);

le_v8js_script = zend_register_list_destructors_ex(v8js_script_dtor, NULL, PHP_V8JS_SCRIPT_RES_NAME, module_number);

Expand Down
51 changes: 27 additions & 24 deletions v8js_convert.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ v8::Local<v8::Value> zend_long_to_v8js(zend_long v, v8::Isolate *isolate) /* {{{

v8::Local<v8::Value> zval_to_v8js(zval *value, v8::Isolate *isolate) /* {{{ */
{
v8js_ctx *ctx = (v8js_ctx *) isolate->GetData(0);
v8::Local<v8::Value> jsValue;
zend_string *value_str;
zend_class_entry *ce;
Expand Down Expand Up @@ -150,19 +151,21 @@ v8::Local<v8::Value> zval_to_v8js(zval *value, v8::Isolate *isolate) /* {{{ */
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();
if (ctx->flags & V8JS_FLAG_CHECK_BINARY_STRING) {
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_FALSE)) {

jsValue = v8::String::NewFromOneByte(isolate, (unsigned char *)ZSTR_VAL(value_str), v8::NewStringType::kNormal, ZSTR_LEN(value_str)).ToLocalChecked();
return jsValue;
}
}

jsValue = V8JS_ZSTR(value_str);

break;

Expand Down Expand Up @@ -204,19 +207,19 @@ int v8js_to_zval(v8::Local<v8::Value> jsValue, zval *return_value, int flags, v8

if (jsValue->IsString())
{
v8::Local<v8::String> strOneByte = jsValue->ToString(v8_context).ToLocalChecked();
if (strOneByte->IsOneByte() && strOneByte->ContainsOnlyOneByte()) {
int length = strOneByte->Length();
char* char_value = new char[length + 1];
strOneByte->WriteOneByte(isolate, reinterpret_cast<uint8_t*>(&char_value[0]), 0, length);

RETVAL_STRINGL(char_value, length);
}
else {
v8::String::Utf8Value str(isolate, jsValue);
const char *cstr = ToCString(str);
RETVAL_STRINGL(cstr, str.length());
if (ctx->flags & V8JS_FLAG_CHECK_BINARY_STRING) {
v8::Local<v8::String> strOneByte = jsValue->ToString(v8_context).ToLocalChecked();
if (strOneByte->ContainsOnlyOneByte()) {
int length = strOneByte->Length();
char* char_value = new char[length + 1];
strOneByte->WriteOneByte(isolate, reinterpret_cast<uint8_t*>(&char_value[0]), 0, length);
RETVAL_STRINGL(char_value, length);
return SUCCESS;
}
}
v8::String::Utf8Value str(isolate, jsValue);
const char *cstr = ToCString(str);
RETVAL_STRINGL(cstr, str.length());
}
else if (jsValue->IsBoolean())
{
Expand Down

4 comments on commit fd3246b

@redbullmarky
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code example on #454 had some of the original checks:

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

is this not necessary any longer?

@redbullmarky
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also - if the mbstring extension is NOT available (it wasn't on my build machine, until I had to test this) will it gracefully fallback to pre-existing behaviour?
Should it be checked and fail gracefully, or should it throw an exception IF the new flag is enabled but the mbstring extension is missing?

@chrisbckr
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the numeric_limits its still necessary.

And if the mbstring extension is not available, call_user_function will return FAILURE and then will fallback the default behaviour.
But its possible to throw an exception if the new flag is enabled but fail to call mb_check_encoding.

@redbullmarky
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the numeric_limits its still necessary.

Ahh I see it's still there, my bad!

But its possible to throw an exception if the new flag is enabled but fail to call mb_check_encoding.

It kind of feels safer this way (i.e. throw an Exception early/at startup if mbstring is not available but the option is enabled). Else you might find unpredictability between two systems (one with, one without) and not know why. You're in no doubt why it's not working if you're told.

Please sign in to comment.