Zend Framework

Zend_Amf_Util_BinaryStream - Endian Detection Issue and Fix proposal

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.7.0, 1.7.1, 1.7.2, 1.7.3, 1.7.4, 1.7.5, 1.7.6, 1.7.7, 1.7.8, 1.8.0, 1.8.1, 1.8.2, 1.8.3, 1.8.4
  • Fix Version/s: 1.9.7
  • Component/s: Zend_Amf
  • Labels:
    None

Description

On the constructor the class variable "$this->_bigEndian" is hardcoded with a "1" value, on 64 Bits processors this convert the float values incorrectly because the method "writeDouble" execute a strrev($stream), with that all the float values are displayed as 0.40998789798-E123 for example

I detected that on the constructor is the following sentence:

$testEndian = unpack("C*", pack("S*", 256));

and following is: $this->_bigEndian = 1;

that I believe that the intention is to detect the endian on the server but it was never used

I changed the line with $this->_bigEndian = 1;

to

$this->_bigEndian = $testEndian[2];

Please verify if the solution is correct, I tested on my both environments:

My Linux box running: Apache/2.0.59 (Unix) DAV/2 PHP/5.2.6

My Solaris Box 64bits: Apache/2.0.59 (Unix) PHP/5.2.9

Let me know if you need more details and If I can help documenting this

  1. patch.txt
    06/Jan/10 12:54 PM
    5 kB
    Marc Bennewitz (private)
  2. story.txt
    06/Jan/10 4:21 PM
    0.3 kB
    Alejandro Santos
  3. testdox.txt
    06/Jan/10 4:21 PM
    8 kB
    Alejandro Santos

Activity

Hide
Marc Bennewitz (private) added a comment -

I don't know how your change will work correctly because on readDouble the byte order wasn't tested before reverting the stream.
Additionally this will set bigEndian to 1 on little endian systems.

Show
Marc Bennewitz (private) added a comment - I don't know how your change will work correctly because on readDouble the byte order wasn't tested before reverting the stream. Additionally this will set bigEndian to 1 on little endian systems.
Hide
Alejandro Santos added a comment -

I tested on both environments little endian and big endian, we have been working that way for months and we didn't had a problem

Show
Alejandro Santos added a comment - I tested on both environments little endian and big endian, we have been working that way for months and we didn't had a problem
Hide
Marc Bennewitz (private) added a comment -

I added a patch.

Can you run Zend_Amf_AllTests with the applied patch and test if this working for you please.

thx

Show
Marc Bennewitz (private) added a comment - I added a patch. Can you run Zend_Amf_AllTests with the applied patch and test if this working for you please. thx
Hide
Alejandro Santos added a comment -

Hi Marc, I have patched and executed the all tests for AMF and got no errors.

Show
Alejandro Santos added a comment - Hi Marc, I have patched and executed the all tests for AMF and got no errors.
Hide
Marc Bennewitz (private) added a comment -

fixed in r20125

Show
Marc Bennewitz (private) added a comment - fixed in r20125
Hide
Alejandro Santos added a comment -

Great, Thanks Marc, there is anything else that I have to do?

Show
Alejandro Santos added a comment - Great, Thanks Marc, there is anything else that I have to do?
Hide
Wade Arnold added a comment -

I see that the patch was committed to trunk but there was no test case commit to trunk. Do you plan on committing a test?

Show
Wade Arnold added a comment - I see that the patch was committed to trunk but there was no test case commit to trunk. Do you plan on committing a test?
Hide
Wade Arnold added a comment -

Also thanks for the patch!!!

Show
Wade Arnold added a comment - Also thanks for the patch!!!
Hide
Wade Arnold added a comment -

I have run AllTests with this patch on 64 and 32 bit system and everything passes. We should really add a patch for when it would fail so that people know to upgrade. I am going to promote the change to the next release.

Show
Wade Arnold added a comment - I have run AllTests with this patch on 64 and 32 bit system and everything passes. We should really add a patch for when it would fail so that people know to upgrade. I am going to promote the change to the next release.
Hide
Marc Bennewitz (private) added a comment -

Hi Wade,

This hadn't do anything with 32/64bit but with little/big endian systems.

I haven't got an big-endian (e.g Solaris, SPARC) system to test it by my self.
But after viewing i think the tests didn't fail on big endian systems.

Show
Marc Bennewitz (private) added a comment - Hi Wade, This hadn't do anything with 32/64bit but with little/big endian systems. I haven't got an big-endian (e.g Solaris, SPARC) system to test it by my self. But after viewing i think the tests didn't fail on big endian systems.
Hide
Wade Arnold added a comment -

I personally don't have access to anything without intel inside so thanks for making the patch!

Show
Wade Arnold added a comment - I personally don't have access to anything without intel inside so thanks for making the patch!

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: