Thread
-
Re: [HACKERS] Open 6.4 items
Tom Lane <tgl@sss.pgh.pa.us> — 1998-10-09T15:01:29Z
Tom Ivar Helbekkmo <tih@nhh.no> writes: > Now I'm explicitly asking for at least one byte more than I need, and > I'm pretty damn sure that I never touch that extra byte, but something > seems to, since the problem goes away. It's arrogant of me, I know, > but barring a complete misunderstanding on my part of how variable > size records work (or a stupid bug that I've been staring at for hours > without seeing, of course), I'd say that something outside my code is > at fault. Any ideas as to how to try to find out? Well, I hate to ruin your day, but coming pre-armed with the knowledge that the code is writing one byte too many, it's pretty obvious that the first loop in inet_net_pton_ipv4 does indeed do that. Specifically at else if (size-- > 0) *++dst = 0, dirty = 0; where, when size (the number of remaining destination bytes) is reduced to 0, the code nonetheless advances dst and clears the next byte. The loop logic is fundamentally faulty: you can't check for emsgsize overflow until you get a digit that is supposed to go into another byte. I'd try something like tmp = 0; ndigits = 0; // ndigits is # of hex digits seen for cur byte while (ch = next hex digit) { n = numeric equivalent of ch; assert(n >= 0 && n <= 15); tmp = (tmp << 4) | n; if (++ndigits == 2) { if (size-- <= 0) goto emsgsize; *dst++ = (u_char) tmp; tmp = 0, ndigits = 0; } } if (ndigits) goto enoent; // odd number of hex digits is bogus BTW, shouldn't this routine clear out all "size" bytes of the destination, even if the given data doesn't fill it all? A memset at the top might be worthwhile. regards, tom lane -
Re: [HACKERS] Open 6.4 items
Tom Ivar Helbekkmo <tih@nhh.no> — 1998-10-09T20:38:37Z
Tom Lane <tgl@sss.pgh.pa.us> writes: > Well, I hate to ruin your day, You sure didn't! You made my day! :-) > but coming pre-armed with the knowledge > that the code is writing one byte too many, it's pretty obvious that the > first loop in inet_net_pton_ipv4 does indeed do that. Specifically at > else if (size-- > 0) > *++dst = 0, dirty = 0; > where, when size (the number of remaining destination bytes) is reduced > to 0, the code nonetheless advances dst and clears the next byte. You're quite right, and I should have caught this one myself, only I must have goofed when I tested the function. (It is written in a style that's just obscure enough that I chose testing it instead of studying it until I understood in detail what it did.) As far as I can tell, the only actual error in Paul Vixie's code is that the two lines you quote above should be: else if (--size > 0) *++dst = 0, dirty = 0; That is, size should be predecremented instead of postdecremented. I'm cc-ing Paul on this, as I assume he wants to get the fix into BIND, which is where inet_net_ntop() and inet_net_pton() came from. > BTW, shouldn't this routine clear out all "size" bytes of the > destination, even if the given data doesn't fill it all? A memset > at the top might be worthwhile. It does: there is code further down that handles that. Another example of the obscure programming style: this function really shows what a low-level language C is! :-) -tih -- Popularity is the hallmark of mediocrity. --Niles Crane, "Frasier"
-
Re: [HACKERS] Open 6.4 items
Tom Ivar Helbekkmo <tih@nhh.no> — 1998-10-10T12:12:40Z
I wrote: > As far as I can tell, the only actual error in Paul Vixie's code is > that the two lines you quote above should be: > > else if (--size > 0) > *++dst = 0, dirty = 0; That is, the patch to apply to fix the INET type is: Index: inet_net_pton.c =================================================================== RCS file: /usr/local/cvsroot/pgsql/src/backend/utils/adt/inet_net_pton.c,v retrieving revision 1.2 diff -r1.2 inet_net_pton.c 120c120 < else if (size-- > 0) --- > else if (--size > 0) -tih -- Popularity is the hallmark of mediocrity. --Niles Crane, "Frasier"
-
Re: [HACKERS] Open 6.4 items
Bruce Momjian <maillist@candle.pha.pa.us> — 1998-10-12T01:31:06Z
Applied > I wrote: > > > As far as I can tell, the only actual error in Paul Vixie's code is > > that the two lines you quote above should be: > > > > else if (--size > 0) > > *++dst = 0, dirty = 0; > > > That is, the patch to apply to fix the INET type is: > > Index: inet_net_pton.c > =================================================================== > RCS file: /usr/local/cvsroot/pgsql/src/backend/utils/adt/inet_net_pton.c,v > retrieving revision 1.2 > diff -r1.2 inet_net_pton.c > 120c120 > < else if (size-- > 0) > --- > > else if (--size > 0) > > -tih > -- > Popularity is the hallmark of mediocrity. --Niles Crane, "Frasier" > > -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
-
RE: [HACKERS] Open 6.4 items
Taral <taral@mail.utexas.edu> — 1998-10-12T02:16:17Z
> Applied > > > I wrote: > > > > > As far as I can tell, the only actual error in Paul Vixie's code is > > > that the two lines you quote above should be: > > > > > > else if (--size > 0) > > > *++dst = 0, dirty = 0; [snip] > > < else if (size-- > 0) > > --- > > > else if (--size > 0) Didn't we agree this WASN'T the fix for the problem? Something about return values? I seem to remember that this piece of code needed to be rewritten... Taral
-
Re: [HACKERS] Open 6.4 items
Thomas Lockhart <lockhart@alumni.caltech.edu> — 1998-10-12T06:03:22Z
> Didn't we agree this WASN'T the fix for the problem? Something about > return values? I seem to remember that this piece of code needed to be > rewritten... Yup. Bruce, Tom has another patch he posted which will behave better... - Tom -
Re: [HACKERS] Open 6.4 items
Bruce Momjian <maillist@candle.pha.pa.us> — 1998-10-12T13:47:39Z
> > Didn't we agree this WASN'T the fix for the problem? Something about > > return values? I seem to remember that this piece of code needed to be > > rewritten... > > Yup. Bruce, Tom has another patch he posted which will behave better... > > - Tom > I have lost it. Could someone resend it, and the patch to reverse too. Thanks. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
-
Re: [HACKERS] Open 6.4 items
Tom Ivar Helbekkmo <tih@nhh.no> — 1998-10-12T15:16:41Z
Bruce Momjian <maillist@candle.pha.pa.us> writes: > I have lost it. Could someone resend it, and the patch to reverse too. This is the one that shouldn't have been applied: Index: inet_net_pton.c =================================================================== RCS file: /usr/local/cvsroot/pgsql/src/backend/utils/adt/inet_net_pton.c,v retrieving revision 1.2 diff -r1.2 inet_net_pton.c 120c120 < else if (size-- > 0) --- > else if (--size > 0) This is the one that works: Index: inet_net_pton.c =================================================================== RCS file: /usr/local/cvsroot/pgsql/src/backend/utils/adt/inet_net_pton.c,v retrieving revision 1.2 diff -r1.2 inet_net_pton.c 108c108,109 < *dst = 0, dirty = 0; --- > tmp = 0; > dirty = 0; 117,122c118,127 < *dst |= n; < if (!dirty++) < *dst <<= 4; < else if (size-- > 0) < *++dst = 0, dirty = 0; < else --- > tmp = (tmp << 4) | n; > if (++dirty == 2) { > if (size-- <= 0) > goto emsgsize; > *dst++ = (u_char) tmp; > tmp = 0, dirty = 0; > } > } > if (dirty) { > if (size-- <= 0) 123a129,130 > tmp <<= 4; > *dst++ = (u_char) tmp; 125,126d131 < if (dirty) < size--; -tih -- Popularity is the hallmark of mediocrity. --Niles Crane, "Frasier" -
Re: [HACKERS] Open 6.4 items
Bruce Momjian <maillist@candle.pha.pa.us> — 1998-10-12T15:57:13Z
Both applied, with the first one reverse applied. Thanks. > Bruce Momjian <maillist@candle.pha.pa.us> writes: > > > I have lost it. Could someone resend it, and the patch to reverse too. > > This is the one that shouldn't have been applied: > > Index: inet_net_pton.c > =================================================================== > RCS file: /usr/local/cvsroot/pgsql/src/backend/utils/adt/inet_net_pton.c,v > retrieving revision 1.2 > diff -r1.2 inet_net_pton.c > 120c120 > < else if (size-- > 0) > --- > > else if (--size > 0) > > This is the one that works: > > Index: inet_net_pton.c > =================================================================== > RCS file: /usr/local/cvsroot/pgsql/src/backend/utils/adt/inet_net_pton.c,v > retrieving revision 1.2 > diff -r1.2 inet_net_pton.c > 108c108,109 > < *dst = 0, dirty = 0; > --- > > tmp = 0; > > dirty = 0; > 117,122c118,127 > < *dst |= n; > < if (!dirty++) > < *dst <<= 4; > < else if (size-- > 0) > < *++dst = 0, dirty = 0; > < else > --- > > tmp = (tmp << 4) | n; > > if (++dirty == 2) { > > if (size-- <= 0) > > goto emsgsize; > > *dst++ = (u_char) tmp; > > tmp = 0, dirty = 0; > > } > > } > > if (dirty) { > > if (size-- <= 0) > 123a129,130 > > tmp <<= 4; > > *dst++ = (u_char) tmp; > 125,126d131 > < if (dirty) > < size--; > > -tih > -- > Popularity is the hallmark of mediocrity. --Niles Crane, "Frasier" > -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026