Fixed #74063 - NumberFormatter did not work properly when restored from session. #2378

andrewnester · 2017-02-09T15:00:10Z

Bug fix for https://bugs.php.net/bug.php?id=74063

…om session.

smalyshev · 2017-02-09T18:07:22Z

ext/intl/formatter/formatter_main.c

+        }
+
+		/* Create an ICU number formatter. */
+    	FORMATTER_OBJECT(nfo) = unum_open(style, spattern, spattern_len, locale, NULL, &INTL_DATA_ERROR_CODE(nfo));


what if this fails?

The same call we have in numfmt_ctor and I guess in case if this call will fail, wrong number formatter object will be handled by FORMATTER_METHOD_FETCH_OBJECT macro

smalyshev · 2017-02-09T18:10:22Z

ext/intl/formatter/formatter_main.c

+        }
+
+		/* Create an ICU number formatter. */
+    	FORMATTER_OBJECT(nfo) = unum_open(style, spattern, spattern_len, locale, NULL, &INTL_DATA_ERROR_CODE(nfo));


If pattern is not set or is not a string, this will be called with null pattern and probably will crash. Also, style may not be initialized when calling it. Also, I don't think default locale should be used when unserializing - this may lead to a situation where same object ins unserialized in different ways, leading to very hard to reproduce bugs.

@smalyshev case when pattern is null covered with test provided, it works ok.

About style - agree, it's better at least to initialize it with 0

And good catch about default locate, thanks! I will fix it.

@smalyshev just fixed it, thanks!

nikic · 2017-02-12T12:15:04Z

ext/intl/formatter/formatter_main.c

+			if (z_locale && Z_TYPE_P(z_locale) == IS_STRING) {
+				str = zval_get_string(z_locale);
+				locale = ZSTR_VAL(str);
+				zend_string_release(str);


This releases the string to which locale still points (and is used later).

@nikic thanks for the review! just fixed all the issues.

nikic · 2017-02-12T12:16:14Z

ext/intl/formatter/formatter_main.c

+			str = zval_get_string(z_pattern);
+			pattern = ZSTR_VAL(str);
+			pattern_len = ZSTR_LEN(str);
+			zend_string_release(str);


Same here, this also releases a string that may later be used. Instead you should simply keep the whole zend_string and release it at the end.

nikic · 2017-02-12T12:17:02Z

ext/intl/formatter/formatter_main.c

+
+    	if(spattern) {
+            efree(spattern);
+        }


Something went wrong with the indentation here, looks like mix of tabs and spaces.

krakjoe · 2017-02-13T16:58:35Z

@andrewnester I see commits yesterday, status please ?

andrewnester · 2017-02-13T17:00:35Z

@krakjoe sorry, changes requested by @nikic fixed, mentioned it here #2378 (comment)

smalyshev · 2017-02-13T21:49:01Z

ext/intl/formatter/formatter_main.c

+
+		z_locale = zend_read_property(Z_OBJCE_P(return_value), return_value, "locale", sizeof("locale") - 1, 0, &rv);
+		if (z_locale != NULL) {
+			zstr_locale = zval_get_string(z_locale);


Since it's unserialized data, there's no guarantee z_locale is a string. It should be checked, and wakeup should fail if it's not.

This doesn't assume that z_locale is a string: It does a string cast.

ah, well, I'm not sure it's the right thing to do anyway - what if it's an object? It's probably not safe to call __toString here... - but it's better then. For long conversion, it may be ok, not sure... I'd not risk it anyway and refuse to deal with broken data, we had enough problem with it lately.

I agree with @smalyshev - it would be better to check for types and fail waking up in case of wrong data

smalyshev · 2017-02-13T21:49:20Z

ext/intl/formatter/formatter_main.c

+ */
+PHP_METHOD( NumberFormatter, __wakeup )
+{
+		const char* locale = intl_locale_get_default();


still uses default locale if serialization data do not contain locale value. Instead, it should fail.

smalyshev · 2017-02-13T21:49:38Z

ext/intl/formatter/formatter_main.c

+
+		z_style = zend_read_property(Z_OBJCE_P(return_value), return_value, "style", sizeof("style") - 1, 0, &rv);
+		if (z_style != NULL) {
+			style = zval_get_long(z_style);


Same here, should not assume it's long.

smalyshev · 2017-02-13T21:50:03Z

ext/intl/formatter/formatter_main.c

+
+		z_pattern = zend_read_property(Z_OBJCE_P(return_value), return_value, "pattern", sizeof("pattern") - 1, 0, &rv);
+		if (z_pattern != NULL) {
+			zstr_pattern = zval_get_string(z_pattern);


Same here, should not assume it's string.

smalyshev · 2017-02-13T21:50:25Z

ext/intl/formatter/formatter_main.c

+
+		/* Convert pattern (if specified) to UTF-16. */
+		if(pattern && pattern_len) {
+			intl_convert_utf8_to_utf16(&spattern, &spattern_len, pattern, pattern_len, &INTL_DATA_ERROR_CODE(nfo));


error code not checked here

@smalyshev thanks! just made changes you requested.

…tter instance

nikic · 2017-02-14T15:26:29Z

My main question here would be what our current stance on serialization of internal classes is in general. This particular PR uses an approach I haven't seen anywhere, which is to actually mirror the internal object state with exposed properties. The two approaches that are used elsewhere are a) using a get_properties handler to expose additional properties for serialization or b) use the Serializable interface. Both options are not idea, because get_properties leaves behind the additional properties, which confuses people (see ext/date), while Serializable always requires some custom code and some care to avoid security issues.

I'm wondering if it might make sense to add a get_properties_for_serialize handler (or similar), which is a variant of get_properties that is used for serialization (and returns a temporary HT). This would allow us to use wakeup-based unserialization without leaking additional (PHP-level) properties.

smalyshev · 2017-02-14T19:40:40Z

I remember seeing this in other classes too, though don't remember the examples right now. In general, (un)serializing internal classes is a problem because not all classes have exportable - and, more importantly, importable - state. I think it makes a lot of sense to have centralized approach to this, and discussing it on internals and probably making the RFC looks like the right way to go here.

andrewnester · 2017-02-14T20:47:40Z

@nikic @smalyshev just to let you know I took this approach from this https://github.com/php/php-src/blob/master/Zend/zend_exceptions.c#L314

andrewnester · 2017-02-26T20:59:56Z

@nikic @smalyshev so what's the conclusion finally on this one?

krakjoe · 2017-03-11T06:55:46Z

@andrewnester it would seem there is some dissent from core maintainers about the suitability of this approach, and in addition, some suggestions for other solutions to the problem being solved here.

Since an RFC has been suggested, and since a discussion with the wider community would appear to be necessary, I would suggest at this point that you start an internals discussion with a view to creating an RFC, and possibly adopting a different approach.

Please link to any discussion here for future reference.

andrewnester · 2017-04-01T12:09:32Z

@krakjoe thanks! I just started discussion here http://news.php.net/php.internals/98706

andrewnester · 2017-05-12T16:22:46Z

@krakjoe I tried to start discussion on internals but doesn't look like it's going to happen

krakjoe · 2017-07-10T06:24:13Z

What I'm going to do is close this PR. What I suggest you do is open a new PR with a new approach, if we (those people that get involved in pull request discussions) find the approach to be acceptable, we may be able to skip the RFC, or it may just be a formality ... either way it would appear that this approach is dead in the water, and a fresh approach is needed.

I'm hoping we can skip an RFC if the approach is okay, personally. Sorry about the amount of time this is taking ...

krakjoe added the Bug label Feb 9, 2017

andrewnester force-pushed the 74063-number-formatter branch from fba6e74 to b45a3dc Compare February 9, 2017 15:42

Fixed #74063 - NumberFormatter did not work properly when restored fr…

5176ccc

…om session.

andrewnester force-pushed the 74063-number-formatter branch from b45a3dc to 5176ccc Compare February 9, 2017 15:59

smalyshev reviewed Feb 9, 2017

View reviewed changes

Fixes for NumberFormatter::__wakeup for proper unserializing

ef8201c

andrewnester force-pushed the 74063-number-formatter branch from 709df96 to ef8201c Compare February 9, 2017 20:58

nikic reviewed Feb 12, 2017

View reviewed changes

Fixed zend_string_release calls and couple of code style fixes

9b57c2b

krakjoe added the Waiting on Author label Feb 13, 2017

smalyshev reviewed Feb 13, 2017

View reviewed changes

Added type checking and error handling when unserializing NumberForma…

89e6556

…tter instance

nikic removed the Waiting on Author label Feb 24, 2017

krakjoe added the Waiting on Author label Mar 11, 2017

krakjoe closed this Jul 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #74063 - NumberFormatter did not work properly when restored from session. #2378

Fixed #74063 - NumberFormatter did not work properly when restored from session. #2378

andrewnester commented Feb 9, 2017

smalyshev Feb 9, 2017

andrewnester Feb 9, 2017

smalyshev Feb 9, 2017

andrewnester Feb 9, 2017

andrewnester Feb 9, 2017

nikic Feb 12, 2017 •

edited

Loading

andrewnester Feb 12, 2017

nikic Feb 12, 2017

nikic Feb 12, 2017

krakjoe commented Feb 13, 2017

andrewnester commented Feb 13, 2017

smalyshev Feb 13, 2017

nikic Feb 13, 2017

smalyshev Feb 13, 2017 •

edited

Loading

andrewnester Feb 14, 2017

smalyshev Feb 13, 2017

smalyshev Feb 13, 2017

smalyshev Feb 13, 2017

smalyshev Feb 13, 2017

andrewnester Feb 14, 2017

nikic commented Feb 14, 2017

smalyshev commented Feb 14, 2017

andrewnester commented Feb 14, 2017

andrewnester commented Feb 26, 2017

krakjoe commented Mar 11, 2017 •

edited

Loading

andrewnester commented Apr 1, 2017

andrewnester commented May 12, 2017

krakjoe commented Jul 10, 2017

Fixed #74063 - NumberFormatter did not work properly when restored from session. #2378

Fixed #74063 - NumberFormatter did not work properly when restored from session. #2378

Conversation

andrewnester commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic Feb 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krakjoe commented Feb 13, 2017

andrewnester commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smalyshev Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic commented Feb 14, 2017

smalyshev commented Feb 14, 2017

andrewnester commented Feb 14, 2017

andrewnester commented Feb 26, 2017

krakjoe commented Mar 11, 2017 • edited Loading

andrewnester commented Apr 1, 2017

andrewnester commented May 12, 2017

krakjoe commented Jul 10, 2017

nikic Feb 12, 2017 •

edited

Loading

smalyshev Feb 13, 2017 •

edited

Loading

krakjoe commented Mar 11, 2017 •

edited

Loading