Skip to content

Fix bug #75237 (jsonSerialize() - Returning new instance of self causes segfault)#2763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
SammyK wants to merge 1 commit into from
Closed

Fix bug #75237 (jsonSerialize() - Returning new instance of self causes segfault) #2763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions ext/json/json_encoder.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -483,6 +483,14 @@ static int php_json_encode_serializable_object(smart_str *buf, zval *val, int op
}

if ((Z_TYPE(retval) == IS_OBJECT) &&
(Z_OBJ(retval) != Z_OBJ_P(val)) &&
ce->name == Z_OBJCE(retval)->name) {

@rmccullaghrmccullaghSep 21, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are you trying to compare strings here? ce->name is a zend_string, so if you want to compare the char values, == will only compare the addresses.

@SammyKSammyKSep 24, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Nice catch! So if I wanted to compare zend_string's, would I change it to something like this: ZSTR_VAL(ce->name) == ZSTR_VAL(Z_OBJCE(retval)->name)?

@rmccullaghrmccullaghSep 24, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could use zend_string_equals, which takes 2 zend_string * arguments. So I believe you could do: zend_string_equals(ce->name, Z_OBJCE(retval)->name) I'm not sure what Z_OBJCE(retval)->name expands to? Is it a zend_string *. or a char *?

@SammyKSammyKSep 25, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oh awesome - thanks so much! :)

@jhdxrjhdxrSep 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@rmccullagh it's a zend_string *, Z_OBJCE returns a _zend_class_entry, and you can find the definition of the structure here. Anyway, I recommend zend_string_equals as well. If there is a wrapper method, then use it.

@sgolemonsgolemonOct 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Already merged, but I just watched your video so here's some thoughts:

  1. If you're just trying to match the class, you can compare the ce only. This will be more correct than comparing names and (nominally) more performant. if ((Z_TYPE_P(retval) == IS_OBJECT) && (Z_OBJCE_P(retval) == ce)) { ... }

  2. The segfault is still easily triggerable via this script. As you mention in your video, there's a much bigger task involved in fixing this properly.

class A implements \JsonSerializable {
publicfunctionjsonSerialize() {
returnnewB;
}
}
class B implements \JsonSerializable {
publicfunctionjsonSerialize() {
returnnewA;
}
}

@jhdxrjhdxrOct 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@sgolemon this pr has been closed without merge. and the example is exactly what I gave here. BTW, what do you mean by video? I don't see any video mentioned here

@SammyKSammyKOct 10, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks @sgolemon! I'll update my blog with your comment. :)

@jhdxr I made a little screencast of how I found & patched this bug. :)

@sgolemonsgolemonOct 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@jhdxr Oh, heh. I should really read the conversation first. I was only looking at the code review comments and interpreted the "Closed" as merged because we don't really merge via github. :p

zend_throw_exception_ex(NULL, 0, "%s::jsonSerialize() cannot return a new instance of itself", ZSTR_VAL(ce->name));
zval_ptr_dtor(&retval);
zval_ptr_dtor(&fname);
PHP_JSON_HASH_APPLY_PROTECTION_DEC(myht);
return FAILURE;
} else if ((Z_TYPE(retval) == IS_OBJECT) &&
(Z_OBJ(retval) == Z_OBJ_P(val))) {
/* Handle the case where jsonSerialize does: return $this; by going straight to encode array */
PHP_JSON_HASH_APPLY_PROTECTION_DEC(myht);
Expand Down
20 changes: 20 additions & 0 deletions ext/json/tests/bug75237.phpt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
--TEST--
Bug #75237 (jsonSerialize() - Returning new instance of self causes segfault)
--SKIPIF--
<?php if (!extension_loaded("json")) die("skip ext/json required"); ?>
--FILE--
<?php
class Foo implements JsonSerializable {
public function jsonSerialize() {
return new self;
}
}

try {
var_dump(json_encode(new Foo));
} catch (Exception $e) {
echo $e->getMessage();
}
?>
--EXPECT--
Foo::jsonSerialize() cannot return a new instance of itself