BUG #5237: strange int->bit and bit->int conversions

The following bug has been logged online:

Bug reference: 5237
Logged by: Roman Kononov
Email address: kononov(at)ftml(dot)net
PostgreSQL version: 8.4.1
Operating system: GNU/Linux x86_64
Description: strange int->bit and bit->int conversions
Details:

test=# select (11::int4<<23 | 11::int4)::bit(32);
00000101100000000000000000001011

test=# select (11::int4<<23 | 11::int4)::bit(33);
000001011100000000000000000001011

test=# select (11::int4<<23 | 11::int4)::bit(39);
000001010000101100000000000000000001011

test=# select (11::int4<<23 | 11::int4)::bit(40);
0000000000000101100000000000000000001011

The ::bit(33) and ::bit(39) conversions seem wrong.

test-std=# select 1::int4::bit(32)::int4;
1

test-std=# select 1::int4::bit(33)::int4;
ERROR: integer out of range

Why is it out of range?

The bitfromint8() and bitfromint4() are hosed. They produce wrong
results when the BIT size is more than 64 and 32 respectively, and the
BIT size is not multiple of 8, and the most significant byte of the
integer value is not 0x00 or 0xff.

For example:

test=# select (11::int4<<23 | 11::int4)::bit(32);
00000101100000000000000000001011

test=# select (11::int4<<23 | 11::int4)::bit(33);
000001011100000000000000000001011

test=# select (11::int4<<23 | 11::int4)::bit(39);
000001010000101100000000000000000001011

test=# select (11::int4<<23 | 11::int4)::bit(40);
0000000000000101100000000000000000001011

The ::bit(33) and ::bit(39) conversions are wrong.

The patch re-implements the conversion functions.

Roman Kononov <kononov(at)ftml(dot)net> writes:
> The bitfromint8() and bitfromint4() are hosed. They produce wrong
> results when the BIT size is more than 64 and 32 respectively, and the
> BIT size is not multiple of 8, and the most significant byte of the
> integer value is not 0x00 or 0xff.

Hm, you're right, it's definitely busted ...

> The patch re-implements the conversion functions.

... but I don't much care for this patch. It's unreadable, uncommented,
and doesn't even try to follow Postgres coding conventions.

It looks to me like the actual bug is that the "first fractional byte"
case ought to shift by destbitsleft - 8 not srcbitsleft - 8. A
secondary problem (which your patch fails to fix) is that if the
compiler chooses to implement signed >> as zero-fill rather than
sign-bit-fill (which it is allowed to do per C99) then we'll get
wrong, or at least not the desired, results. So I arrive at
the attached patch.

Unfortunately we've just missed the window for 8.4.2 etc, but I'll
get this committed for the next updates.

regards, tom lane

Woops, forgot to copy the list.

On Sat, Dec 12, 2009 at 2:15 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Dec 12, 2009 at 2:00 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Roman Kononov <kononov(at)ftml(dot)net> writes:
>>> The bitfromint8() and bitfromint4() are hosed. They produce wrong
>>> results when the BIT size is more than 64 and 32 respectively, and the
>>> BIT size is not multiple of 8, and the most significant byte of the
>>> integer value is not 0x00 or 0xff.
>>
>> Hm, you're right, it's definitely busted ...
>>
>>> The patch re-implements the conversion functions.
>>
>> ... but I don't much care for this patch.  It's unreadable, uncommented,
>> and doesn't even try to follow Postgres coding conventions.
>>
>> It looks to me like the actual bug is that the "first fractional byte"
>> case ought to shift by destbitsleft - 8 not srcbitsleft - 8.  A
>> secondary problem (which your patch fails to fix) is that if the
>> compiler chooses to implement signed >> as zero-fill rather than
>> sign-bit-fill (which it is allowed to do per C99) then we'll get
>> wrong, or at least not the desired, results.  So I arrive at
>> the attached patch.
>
> I'm not sure this fixes it, although I haven't tested.  When we take
> the /* store first fractional byte */ branch, destbitsleft is between
> 1 and 7 bits greater than srcbitsleft.  We then subtract 8 from
> destbitsleft, and the comment on the next line now asserts that the
> two are equal.  That doesn't seem right.
>
> Also, I thought about the sign extension problem, but aren't we
> chopping those bits off anyway on the next line?
>
> ...Robert
>

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I'm not sure this fixes it, although I haven't tested. When we take
>> the /* store first fractional byte */ branch, destbitsleft is between
>> 1 and 7 bits greater than srcbitsleft. We then subtract 8 from
>> destbitsleft, and the comment on the next line now asserts that the
>> two are equal. That doesn't seem right.

The comment's a bit bogus. What it should say, probably, is that the
*correct* value of srcbitsleft is now equal to destbitsleft so we aren't
bothering to track it anymore. If we were being fully anal we'd have
reduced srcbitsleft by some number less than 8 inside the if-branch.

>> Also, I thought about the sign extension problem, but aren't we
>> chopping those bits off anyway on the next line?

Nope, we'd have already stored the wrong bits into the output byte.

regards, tom lane