Thread: oid failures on Alpha solved

Hi,
 It turns out the problem causing the oid failures is with our 
snprintf. specifically we are formatting "%u" incorrectly:

using a enhanced-for-testing version of our snprintf I get.

formatting '-1040' with '%lu' snprintf = 18446744073709550576 sprintf  = 18446744073709550576

formatting '-1040' with '%u' snprintf = 18446744073709550576 sprintf  = 4294966256


oidout() is where the offending call originates, FWIW. snprintf(result, 12, "%u", o);

I've massaged in the snprintf.c code from openssh into postgres, and 
oid now passes the regression test, but have a couple of questions: 1) could the openssh code be a candidate to replace
ourversion? It     looks quite a bit more 'featureful', and I'd imagine it is about     as safe as snprintf gets. 2) do
we_need_ oidout() to "%u", or could we "%lu" and fully take    advantage of the longer long on 64bit platforms?
 

cheers. brent

Brent Verner <brent@rcfile.org> writes:
> formatting '-1040' with '%u'
>   snprintf = 18446744073709550576
>   sprintf  = 4294966256

> oidout() is where the offending call originates, FWIW.
>   snprintf(result, 12, "%u", o);

Hm.  This is clearly a bug in snprintf.  Did I understand you correctly
that configure is choosing backend/port/snprintf.c rather than one from
the C library?  If so, it should be straightforward to fix.

>   1) could the openssh code be a candidate to replace our version? It 
>      looks quite a bit more 'featureful', and I'd imagine it is about 
>      as safe as snprintf gets.

(a) what's the license?  (b) is it a lot bigger than the one we have?

>   2) do we _need_ oidout() to "%u", or could we "%lu" and fully take
>      advantage of the longer long on 64bit platforms?

OID is 4 bytes and %u is the correct format for it.  8-byte OIDs are a
can of worms that we will *not* open just now.
        regards, tom lane

> Brent Verner <brent@rcfile.org> writes:
>> formatting '-1040' with '%u'
>> snprintf = 18446744073709550576
>> sprintf  = 4294966256

>> oidout() is where the offending call originates, FWIW.
>> snprintf(result, 12, "%u", o);

> Hm.  This is clearly a bug in snprintf.

OK, I was able to duplicate this failure on Debian Alpha after forcing
our snprintf emulation to be used instead of the system's.  Problem is
sloppy signed-vs-unsigned casting in snprintf.c.  I've committed a fix.
Another day, another bug...
        regards, tom lane

On 30 Dec 2000 at 12:57 (-0500), Tom Lane wrote:
| Brent Verner <brent@rcfile.org> writes:
| > formatting '-1040' with '%u'
| >   snprintf = 18446744073709550576
| >   sprintf  = 4294966256
| 
| > oidout() is where the offending call originates, FWIW.
| >   snprintf(result, 12, "%u", o);
| 
| Hm.  This is clearly a bug in snprintf.  Did I understand you correctly
| that configure is choosing backend/port/snprintf.c rather than one from
| the C library?  If so, it should be straightforward to fix.

yes, backend/port/snprintf.c is being used.

| >   1) could the openssh code be a candidate to replace our version? It 
| >      looks quite a bit more 'featureful', and I'd imagine it is about 
| >      as safe as snprintf gets.
| 
| (a) what's the license?  (b) is it a lot bigger than the one we have?

a) no license in file. file is obviously 'related' to the current  version used in postgresql. version I used came with
openssh-2.?, so if any license is applicable, i'd imagine it's BSD-like.
 
b) 10.7k postgresql snprintf.c  18.4k openssh snprintf.c  (a sizable portion of this size difference is a test main()
atthe   end of the file openssh file, which I found useful)
 

you can snag a slightly modified-to-work-in-postgres copy from 
http://pg.rcfile.org/alpha/snprintf.c if you'd like to check it out.
it is a drop in replacement as it stands. my changes to the original
were s/long double/double/g and moving some #cpp directives around.

| >   2) do we _need_ oidout() to "%u", or could we "%lu" and fully take
| >      advantage of the longer long on 64bit platforms?
| 
| OID is 4 bytes and %u is the correct format for it.  8-byte OIDs are a
| can of worms that we will *not* open just now.

right on.

cheers. brent

On 30 Dec 2000 at 14:24 (-0500), Tom Lane wrote:
| > Brent Verner <brent@rcfile.org> writes:
| >> formatting '-1040' with '%u'
| >> snprintf = 18446744073709550576
| >> sprintf  = 4294966256
| 
| >> oidout() is where the offending call originates, FWIW.
| >> snprintf(result, 12, "%u", o);
| 
| > Hm.  This is clearly a bug in snprintf.
| 
| OK, I was able to duplicate this failure on Debian Alpha after forcing
| our snprintf emulation to be used instead of the system's.  Problem is
| sloppy signed-vs-unsigned casting in snprintf.c.  I've committed a fix.
| Another day, another bug...

indeed. the most recent snprintf.c works as expected.

thank you. brent