Fix for bug #63437

12 years ago by Anatol Belski — view source

unread

Hi,

I've reworked the patch from
http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff
(mentioned by tony2001) for bug #63437, that seems to fix the issue. That
patch was ported back to 5.3 and adapted to the current 5.4+. Both
variants are posted to the ticket.

Also the test for bug #52113 delivers more plausible results and should be
fixed when the patch is applied.

Regards

Anatol

12 years ago by Anatol Belski — view source

unread

Sorry, the correct one is bug #53437 ...

Hi,

I've reworked the patch from
http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff
(mentioned by tony2001) for bug #63437, that seems to fix the issue. That
patch was ported back to 5.3 and adapted to the current 5.4+. Both variants
are posted to the ticket.

Also the test for bug #52113 delivers more plausible results and should
be fixed when the patch is applied.

Regards

Anatol

12 years ago by Anatol Belski — view source

unread

Hi,

I wanted just remind about that bug and the patches hanging there. If
there are no objections, I'd commit first to 5.5+ next week. 5.3/5.4 were
also debatable.

Regards

Anatol

Sorry, the correct one is bug #53437 ...

Hi,

I've reworked the patch from
http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff
(mentioned by tony2001) for bug #63437, that seems to fix the issue.
That
patch was ported back to 5.3 and adapted to the current 5.4+. Both
variants are posted to the ticket.

Also the test for bug #52113 delivers more plausible results and should
be fixed when the patch is applied.

Regards

Anatol

12 years ago by Derick Rethans — view source

unread

I've reworked the patch from
http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff
(mentioned by tony2001) for bug #63437, that seems to fix the issue.
That patch was ported back to 5.3 and adapted to the current 5.4+.
Both variants are posted to the ticket.

Serializing this as a base64 encoded variant of some binary data is not
a good thing. If you want to serialize, it needs to output the same
thigns that allow users to create the period or interval. I also don't
think this would work for Big Endian vs Little Endian either.

cheers,
Derick

12 years ago by Gustavo Lopes — view source

unread

I've reworked the patch from
http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff
(mentioned by tony2001) for bug #63437, that seems to fix the issue.
That patch was ported back to 5.3 and adapted to the current 5.4+.
Both variants are posted to the ticket.

Serializing this as a base64 encoded variant of some binary data is not
a good thing. If you want to serialize, it needs to output the same
thigns that allow users to create the period or interval.

I would agree in principle, but, as I explained before, there is a
problem. The DatePeriod class has 64-bit integers in its internal
structure. The PHP integer type cannot (in general) represent that data.
So the general method of getting the object data via get_properties and
serializing (and then using __set_state to convert the array back) does
not work unless you represent those 64-bit integers with some non-integer
type and do the conversions.

I also don't think this would work for Big Endian vs Little Endian
either.

It does; the integers are converted to network order before being stored,
so you can share the serialized data between machines with different
endianness.

--
Gustavo Lopes

12 years ago by Anatol Belski — view source

unread

Hi,

On Sat, 2013-03-09 at 21:57 +0100, Gustavo Lopes wrote:

I've reworked the patch from
http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff
(mentioned by tony2001) for bug #63437, that seems to fix the issue.
That patch was ported back to 5.3 and adapted to the current 5.4+.
Both variants are posted to the ticket.

Serializing this as a base64 encoded variant of some binary data is not
a good thing. If you want to serialize, it needs to output the same
thigns that allow users to create the period or interval.

I would agree in principle, but, as I explained before, there is a
problem. The DatePeriod class has 64-bit integers in its internal
structure. The PHP integer type cannot (in general) represent that data.
So the general method of getting the object data via get_properties and
serializing (and then using __set_state to convert the array back) does
not work unless you represent those 64-bit integers with some non-integer
type and do the conversions.
So base64 seems to be only the doubtful point. Thriving to fix that,
what if we could bring it in dependency of libgmp to serialize and read
as strings (and maybe disable serialization otherwise)?

I also don't think this would work for Big Endian vs Little Endian
either.

It does; the integers are converted to network order before being stored,
so you can share the serialized data between machines with different
endianness.

--
Gustavo Lopes

12 years ago by Derick Rethans — view source

unread

On Sat, 2013-03-09 at 21:57 +0100, Gustavo Lopes wrote:

I've reworked the patch from
http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff
(mentioned by tony2001) for bug #63437, that seems to fix the
issue. That patch was ported back to 5.3 and adapted to the
current 5.4+. Both variants are posted to the ticket.

Serializing this as a base64 encoded variant of some binary data
is not a good thing. If you want to serialize, it needs to output
the same thigns that allow users to create the period or interval.

I would agree in principle, but, as I explained before, there is a
problem. The DatePeriod class has 64-bit integers in its internal
structure. The PHP integer type cannot (in general) represent that
data. So the general method of getting the object data via
get_properties and serializing (and then using __set_state to
convert the array back) does not work unless you represent those
64-bit integers with some non-integer type and do the conversions.

So base64 seems to be only the doubtful point. Thriving to fix that,
what if we could bring it in dependency of libgmp to serialize and
read as strings (and maybe disable serialization otherwise)?

Why do you need libgmp for that‽

cheers,
Derick

12 years ago by Anatol Belski — view source

unread

libgmp was just the first shot as it has functions to convert from
arbitrary binary data to string and vice versa, mpz_import and mpz_export.
That's what should work fine across platforms. Looking at the type
definitions here
http://lxr.php.net/xref/PHP_5_5/ext/date/lib/timelib_structs.h#70 i
wouldn't exclude possible platform issues. Implementing that manually is a
tricky job, could be done probably with more homework :)

What is the way you had in the mind to achieve the string<->integer
conversions?

Regards

Anatol

On Sat, 09 Mar 2013 21:36:41 +0100, Derick Rethans derick@php.net
wrote:

I've reworked the patch from
http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff
(mentioned by tony2001) for bug #63437, that seems to fix the
issue. That patch was ported back to 5.3 and adapted to the current
5.4+. Both variants are posted to the ticket.

Serializing this as a base64 encoded variant of some binary data
is not a good thing. If you want to serialize, it needs to output the
same thigns that allow users to create the period or interval.

I would agree in principle, but, as I explained before, there is a
problem. The DatePeriod class has 64-bit integers in its internal
structure. The PHP integer type cannot (in general) represent that
data. So the general method of getting the object data via
get_properties and serializing (and then using __set_state to convert
the array back) does not work unless you represent those 64-bit
integers with some non-integer type and do the conversions.

So base64 seems to be only the doubtful point. Thriving to fix that,
what if we could bring it in dependency of libgmp to serialize and read
as strings (and maybe disable serialization otherwise)?

Why do you need libgmp for that‽

cheers, Derick

12 years ago by Derick Rethans — view source

unread

Please, no top posting!

I would agree in principle, but, as I explained before, there is a
problem. The DatePeriod class has 64-bit integers in its internal
structure. The PHP integer type cannot (in general) represent that
data. So the general method of getting the object data via
get_properties and serializing (and then using __set_state to
convert the array back) does not work unless you represent those
64-bit integers with some non-integer type and do the conversions.

So base64 seems to be only the doubtful point. Thriving to fix
that, what if we could bring it in dependency of libgmp to
serialize and read as strings (and maybe disable serialization
otherwise)?

Why do you need libgmp for that‽

libgmp was just the first shot as it has functions to convert from
arbitrary binary data to string and vice versa, mpz_import and
mpz_export. That's what should work fine across platforms. Looking at
the type definitions here
http://lxr.php.net/xref/PHP_5_5/ext/date/lib/timelib_structs.h#70 i
wouldn't exclude possible platform issues. Implementing that manually
is a tricky job, could be done probably with more homework :)

What is the way you had in the mind to achieve the string<->integer
conversions?

atoll() (or atoq()).

cheers,
Derick

12 years ago by Anatol Belski — view source

unread

On Mon, 2013-03-11 at 11:42 +0000, Derick Rethans wrote:

Please, no top posting!

I would agree in principle, but, as I explained before, there is a
problem. The DatePeriod class has 64-bit integers in its internal
structure. The PHP integer type cannot (in general) represent that
data. So the general method of getting the object data via
get_properties and serializing (and then using __set_state to
convert the array back) does not work unless you represent those
64-bit integers with some non-integer type and do the conversions.

So base64 seems to be only the doubtful point. Thriving to fix
that, what if we could bring it in dependency of libgmp to
serialize and read as strings (and maybe disable serialization
otherwise)?

Why do you need libgmp for that‽

libgmp was just the first shot as it has functions to convert from
arbitrary binary data to string and vice versa, mpz_import and
mpz_export. That's what should work fine across platforms. Looking at
the type definitions here
http://lxr.php.net/xref/PHP_5_5/ext/date/lib/timelib_structs.h#70 i
wouldn't exclude possible platform issues. Implementing that manually
is a tricky job, could be done probably with more homework :)

What is the way you had in the mind to achieve the string<->integer
conversions?

atoll() (or atoq()).

Please take a look at the reworked patch in #53437

https://bugs.php.net/patch-display.php?bug_id=53437&patch=date_patch_var3.patch&revision=latest

Serializing 64 bit integers as strings.

Regards

Anatol

cheers,
Derick

12 years ago by Derick Rethans — view source

unread

On Mon, 2013-03-11 at 11:42 +0000, Derick Rethans wrote:

What is the way you had in the mind to achieve the
string<->integer conversions?

atoll() (or atoq()).

Please take a look at the reworked patch in #53437

https://bugs.php.net/patch-display.php?bug_id=53437&patch=date_patch_var3.patch&revision=latest

Serializing 64 bit integers as strings.

That bit looks ok, I have a few comments though:

PHP_DATE_INTERVAL_READ_PROPERTY("special_type", special.type, unsigned int);
if ((*intobj)->diff->special.amount > 12 || (*intobj)->diff->special.amount < -12) {

  php_error(E_ERROR, "Invalid serialization data for DateInterval object (special_amount)");

}

Checking for the values here, means that the library (timelib) isn't
in control of the values that are allowed anymore. I wonder whether the
deserialisation should bother checking some of those special/extra
types/values. The only important thing is to not have things crash
really.

And secondly, where are the test cases?

cheers,
Derick

--
http://derickrethans.nl | http://xdebug.org
Like Xdebug? Consider a donation: http://xdebug.org/donate.php
twitter: @derickr and @xdebug
Posted with an email client that doesn't mangle email: alpine

12 years ago by Derick Rethans — view source

unread

I've reworked the patch from
http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff
(mentioned by tony2001) for bug #63437, that seems to fix the
issue. That patch was ported back to 5.3 and adapted to the
current 5.4+. Both variants are posted to the ticket.

Serializing this as a base64 encoded variant of some binary data is
not a good thing. If you want to serialize, it needs to output the
same thigns that allow users to create the period or interval.

I would agree in principle, but, as I explained before, there is a
problem. The DatePeriod class has 64-bit integers in its internal
structure. The PHP integer type cannot (in general) represent that
data. So the general method of getting the object data via
get_properties and serializing (and then using __set_state to convert
the array back) does not work unless you represent those 64-bit
integers with some non-integer type and do the conversions.

Then store them as strings or another format that makes it possible to
get a 64bit timestamp back. Of course, I don't think it makes too much
sense to serialize a Period as it's more of a transient iterator
wrapper. For an interval, you'd want to serialise the original input
vectors as well... not an easy task (unless you store the original).

cheers,
Derick