libimobiledevice / libplist

A library to handle Apple Property List format in binary or XML
https://libimobiledevice.org
GNU Lesser General Public License v2.1
546 stars 305 forks source link

Fix wrong length value for uint64's #52

Closed TingPing closed 8 years ago

TingPing commented 9 years ago

The return value of strtoull() is unsigned and 64bits, yet it would set its length to 128bits if it were larger than INT64_MAX (which UINT64_MAX is). So this just always marks it the same (correct) length.

This caused issues later when it asserts that the length is that of uint64_t

TingPing commented 9 years ago

I notice this is also wrong in bplist.c, so I am guessing this is part of the plist spec? It doesn't make sense though since this information is cast away anyway. The other solution is to just change the assert that its always 8 (but still throws away anything larger).

nikias commented 8 years ago

@TingPing it is part of the plist spec. The 128 bit numbers are still treated as 64 bit numbers, however they are used for positive integers that are larger than INT64_MAX (0x7FFFFFFFFFFFFFFF). Take this plist example:

<plist>
<array>
        <integer>18446744073709551615</integer>
        <integer>-1</integer>
        <integer>9223372036854775807</integer>
        <integer>-9223372036854775808</integer>
</array>
</plist>

If you convert this plist after your changes to binary with libplist you will get this output (using Apple's plutil -p on the resulting file):

[
  0 => -1
  1 => -1
  2 => 9223372036854775807
  3 => -9223372036854775808
]

The first number is incorrect since it would need to be stored as a 128 bit number to preserve the sign. This is why in xml_to_node the code will assign a size of 16 or 8 bits depending on the sign and the size of the value.

In node_to_xml, we need to distinguish this case aswell, that's why we use %lli vs. %llu to print the actual value correctly. If the node size is 16 we know that the number is unsigned while a size of 8 denotes a signed value. With your changes you treat all values as unsigned so the above example plist, as correct binary plist, would result in this output (using our plistutil -i):

<plist version="1.0">
<array>
    <integer>18446744073709551615</integer>
    <integer>18446744073709551615</integer>
    <integer>9223372036854775807</integer>
    <integer>9223372036854775808</integer>
</array>
</plist>

The second and the 4th value is incorrect because of this.

Bottom line: Thanks to your report I found another bug which actually caused issues: During binary plist generation the code is supposed to optimize the size of the plist by re-using same-value nodes wherever possible. However for integer nodes the comparison function would just do a uint64_t comparison of two node values, so -1 would have the same value as 18446744073709551615 etc. I fixed this with commit acd226d1f71a78dd23b47a9a5c4ca8cf8068d509.