# Fix for bug #63437

- Thread: https://externals.io/message/66468
- Original archive: https://news-web.php.net/php.internals/66468
- Started by Anatol Belski on 2013-03-05T11:42:12+00:00
- Messages: 12
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #66468 — Anatol Belski — 2013-03-05T11:42:12+00:00

URL: https://externals.io/message/66468#66468

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

---

## #66469 — Anatol Belski — 2013-03-05T11:46:56+00:00

URL: https://externals.io/message/66468#66469 · In reply to: #66468

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

---

## #66566 — Anatol Belski — 2013-03-09T00:32:27+00:00

URL: https://externals.io/message/66468#66566 · In reply to: #66469

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 ...
>
>
> 
>
> > […]

---

## #66567 — Derick Rethans — 2013-03-09T20:36:41+00:00

URL: https://externals.io/message/66468#66567 · In reply to: #66468

> 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

---

## #66569 — Gustavo Lopes — 2013-03-09T20:57:46+00:00

URL: https://externals.io/message/66468#66569 · In reply to: #66567

> 
>
> > […]
>
> 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

---

## #66571 — Anatol Belski — 2013-03-09T22:50:09+00:00

URL: https://externals.io/message/66468#66571 · In reply to: #66569

Hi,

On Sat, 2013-03-09 at 21:57 +0100, Gustavo Lopes wrote: 
> 
> 
> > […]
> 
> 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)?

> 
> > […]
> 
> 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

---

## #66572 — Derick Rethans — 2013-03-09T23:49:15+00:00

URL: https://externals.io/message/66468#66572 · In reply to: #66569

> 
> 
> > […]
> 
> 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

---

## #66573 — Derick Rethans — 2013-03-10T22:11:08+00:00

URL: https://externals.io/message/66468#66573 · In reply to: #66571

> On Sat, 2013-03-09 at 21:57 +0100, Gustavo Lopes wrote: 
> > […]
>
> 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

---

## #66578 — Anatol Belski — 2013-03-11T08:50:12+00:00

URL: https://externals.io/message/66468#66578 · In reply to: #66573

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


> 
>
>
> > […]
>
> Why do you need libgmp for that‽
>
>
> cheers, Derick

---

## #66583 — Derick Rethans — 2013-03-11T11:42:44+00:00

URL: https://externals.io/message/66468#66583 · In reply to: #66578

Please, no top posting!


> 
> > […]

> 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

---

## #66599 — Anatol Belski — 2013-03-13T09:06:06+00:00

URL: https://externals.io/message/66468#66599 · In reply to: #66583

On Mon, 2013-03-11 at 11:42 +0000, Derick Rethans wrote: 
> Please, no top posting!
> 
> 
> > […]
> 
> > […]
> 
> 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

---

## #66610 — Derick Rethans — 2013-03-14T11:42:25+00:00

URL: https://externals.io/message/66468#66610 · In reply to: #66599

> On Mon, 2013-03-11 at 11:42 +0000, Derick Rethans wrote: 
> > […]
> 
> 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