Opened 9 years ago

Closed 9 years ago

#703 closed defect (fixed)

Python 3 sortable_serialise() broken on 32 bit

Reported by: Barry Warsaw Owned by: Olly Betts
Priority: normal Milestone: 1.3.5
Component: Xapian-bindings (Python) Version: 1.3.3
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Operating System: All

Description (last modified by Barry Warsaw)

sortable_serialise() on Python 3 (Xapian 1.3) has two bugs on 32 bit systems. You can see this with the following example:

$ python3
Python 3.5.1+ (default, Jan  5 2016, 11:13:30) 
[GCC 5.3.1 20160101] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from xapian import *
>>> sortable_serialise(14215209984)
OverflowError: Python int too large to convert to C long

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
SystemError: <built-in function sortable_serialise> returned a result with an error set
>>> 

First, the conversion of the int to the C API's double can't handle the 33+-bit size of the value, thus you get the OverflowError. However, sortable_serialise() also doesn't properly check for and handle the exception, but instead returns a non-NULL result which triggers the SystemError once control percolates back up to the eval loop.

Change History (7)

comment:1 by Barry Warsaw, 9 years ago

Description: modified (diff)

comment:2 by Olly Betts, 9 years ago

Component: OtherXapian-bindings (Python)
Milestone: 1.3.x
Owner: changed from Olly Betts to Richard Boulton

Needs fixing before 1.4.0, so setting milestone.

I think I saw some SWIG fixes in this area go in recently, so it might just be a matter of upgrading to a newer SWIG. We probably want to upgrade to a recent SWIG before 1.4.0 anyway.

Which Xapian version are you using, in case that matters? I don't have a 32-bit box to hand to check with.

comment:3 by Barry Warsaw, 9 years ago

We see this currently on Ubuntu Xenial 16.04 since Debian's apt-xapian-index isn't ported to Python 3 yet. In Ubuntu we have Xapian 1.3.3 as the binary package python3-xapian1.3. We haven't upgraded to 1.3.4 yet.

comment:4 by Olly Betts, 9 years ago

Version: 1.3.3

You can probably workaround by converting to float on the Python side, i.e.:

sortable_serialise(float(14215209984))

While that can lose precision for a suitably large number, the C++ function takes a double, so you're going to lose it anyway. Plus this looks like a time_t which isn't going to hit that issue for quite some time.

comment:5 by Olly Betts, 9 years ago

The SWIG fix I had in mind is in the recently released SWIG 3.0.8:

 2015-12-23: ahnolds
             [Python] Fixes for conversion of signed and unsigned integer types:
 
             No longer check for PyInt objects in Python3. Because PyInt_Check
             and friends are #defined to the corresponding PyLong methods, this
             had caused errors in Python3 where values greater than what could be
             stored in a long were incorrectly interpreted as the value -1 with
             the Python error indicator set to OverflowError. This applies to
             both the conversions PyLong->long and PyLong->double.
 
             Conversion from PyLong to long, unsigned long, long long, and
             unsigned long long now raise OverflowError instead of TypeError in
             both Python2 and Python3 for PyLong values outside the range
             expressible by the corresponding C type. This matches the existing
             behavior for other integral types (signed and unsigned ints, shorts,
             and chars), as well as the conversion for PyInt to all numeric
             types. This also indirectly applies to the size_t and ptrdiff_t
             types, which depend on the conversions for unsigned long and long.

comment:6 by Olly Betts, 9 years ago

Owner: changed from Richard Boulton to Olly Betts
Status: newassigned

With 3.4.4, I don't get the SystemError, but I do get b'^' which isn't right - that's the serialisation of -1. With SWIG git master (which is just a few commits after 3.0.8), I get a more plausible answer (I didn't think to check unserialising it again while I still had it built).

We need to check that upgrading SWIG doesn't regress any other languages, but at least we know what the fix for this is now.

comment:7 by Olly Betts, 9 years ago

Milestone: 1.3.x1.3.5
Resolution: fixed
Status: assignedclosed

SWIG version used to bootstrap snapshots and releases updated in [b643e151dc3b906b8ffc6f215e393f1fe5017156].

Note: See TracTickets for help on using tickets.