| Subject: | "DBIS" macro defined incorrectly |
The DBIS macro is declared in the "DBIXS.h" header file and used by the
various DBD::xx modules to fetch the state information published by the
DBI module. The definition of this as follows:
# define DBIS (*(INT2PTR(dbistate_t**, &SvIVX(DBISTATE_ADDRSV))))
(DBIXH.h line 458)
This definition is incorrect.
This bug requires the following preconditions in order to occur:
- Perl built with threads (MULTIPLICITY is defined). Without this, a
different definition for the DBIS macro is used, and that definition
does not exhibit a problem.
- Integers and pointers are not the same size. If integers and pointers
are the same size then the macro becomes a no-op and is harmless.
- Big endian platform. On little endian, everything just works out.
NOTE regarding the severity level I have attached to this ticket:
although most Perl users will not be affected by this bug because they
do not have systems that meet the preconditions above, DBI is completely
unusable due to this bug on those platforms that do meet the
preconditions, so it should be regarded as a critical bug.
Why is this macro wrong?
First we fetch the state variable using DBISTATE_ADDRSV, then we get its
integer value using SvIVX. Now we take a pointer to this integer (using
&). The resulting type is (IV *) but INT2PTR casts it to a
(dbistate_t**). This cast doesn't make any sense: this value is not at
all a pointer to a pointer to a structure of type dbistate_t, it's a
pointer to an integer whose value is the same as the address of a
structure of type dbistate_t.
Here is an example for platform "sun4-solaris-thread-multi-64int" where
the integers are 8 bytes and the pointers are 4 bytes. SvIVX returns an
[8-byte] integer. Now we take a pointer to this integer and cast it to a
(dbistate_t**). Then we dereference that double pointer. This causes us
to look at the first four bytes of the integer and interpret them as a
pointer. Since these are the most significant 4 bytes of the original
integer, the result is incorrect (we are actually trying to get at the
least significant 4 bytes of the integer). We are left with a result of
type (dbistate_t *) which is a NULL pointer.
I do not understand why the definition of this macro is so complicated.
It seems to me that what we are trying to do is actually just this:
#define DBIS (INT2PTR(dbistate_t*, SvIVX(DBISTATE_ADDRSV)))
That is, take the result of SvIVX (which is of type IV) and cast it to a
pointer of type (dbistate_t*). Because of the different sizes of the two
types, this cast causes the most significant bytes of the value to be
discarded on platforms where integers are bigger than pointers, which is
what we want. The resulting (dbistate_t*) is returned directly.
I am mystified as to why the original macro creates an extra indirection
of the pointer only to dereference it again at the end. It is both
simpler and more correct to not do that. I mention my interrogation on
this point only in case there is some reason for the more complicated
definition that eludes me.
TO REPRODUCE:
- Install DBI normally on platform sun4-solaris-thread-multi-64int.
- Unpack, configure, and make the latest version of DBD::mysql.
- make test will fail
TO PROVE MY ALTERNATE DEFINITON OF DBIS WORKS:
- Edit the mysql.xs file that is part of the DBD::mysql distribution.
- Right after the includes at the top of the file, override the
definition of the DBIS macro by inserting these lines:
#undef DBIS
#define DBIS (INT2PTR(dbistate_t*, SvIVX(DBISTATE_ADDRSV)))
- rerun make
- make test now passes
RECOMMENDATION FOR FIX:
Change the definition of the DBIS macro in the DBIXS.h file to the one I
suggest. Change also the dbis macro 2 lines below.
-Phil