Bug 17681
| Summary: | XDR does not handle 64bit data types correctly | ||
|---|---|---|---|
| Product: | Base System | Reporter: | David E. Cross <crossd> |
| Component: | bin | Assignee: | dec |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | ||
| Priority: | Normal | ||
| Version: | 4.0-STABLE | ||
| Hardware: | Any | ||
| OS: | Any | ||
On Thu, 30 Mar 2000 02:11:08 EST, "David E. Cross" wrote:
> http://www.cs.rpi.edu/~crossd/FreeBSD/xdr.diff
Let us know when that server accepts connections to port 80. :-)
Ciao,
Sheldon.
It does now. *sigh* -- David Cross | email: crossd@cs.rpi.edu Acting Lab Director | NYSLP: FREEBSD Systems Administrator/Research Programmer | Web: http://www.cs.rpi.edu/~crossd Rensselaer Polytechnic Institute, | Ph: 518.276.2860 Department of Computer Science | Fax: 518.276.4033 I speak only for myself. | WinNT:Linux::Linux:FreeBSD On Thu, 30 Mar 2000 10:13:24 EST, "David E. Cross" wrote:
> It does now. *sigh*
By the way... You said that your fix is derived from NetBSD. Given
their wild fanaticism for portable code, I'm surprised that your patch
is so much more complicated than theirs.
What gives?
Ciao,
Sheldon.
> > It does now. *sigh* > > By the way... You said that your fix is derived from NetBSD. Given > their wild fanaticism for portable code, I'm surprised that your patch > is so much more complicated than theirs. > > What gives? Hmm? It shouldn't be. The aforementioned patch should make our xdr*() routines look identical to NetBSDs. A patch that I posted earlier to the mailing list was more complicated, and less portable, that is why I switched to the NetBSD version. -- David Cross | email: crossd@cs.rpi.edu Acting Lab Director | NYSLP: FREEBSD Systems Administrator/Research Programmer | Web: http://www.cs.rpi.edu/~crossd Rensselaer Polytechnic Institute, | Ph: 518.276.2860 Department of Computer Science | Fax: 518.276.4033 I speak only for myself. | WinNT:Linux::Linux:FreeBSD On Fri, 31 Mar 2000 11:33:02 EST, "David E. Cross" wrote: > Hmm? It shouldn't be. The aforementioned patch should make our xdr*() > routines look identical to NetBSDs. I'm looking at http://www.cs.rpi.edu/~crossd/FreeBSD/xdr.diff. It results in code that doesn't seem to look anything like the code at http://www.freebsd.org/cgi/cvsweb.cgi/basesrc/lib/libc/rpc/xdr.c?rev=1.21&cvsroot=netbsd . Am I being stupid? Ciao, Sheldon. Hmm.. I feel one of us is probably doing something stupid :)
This is a bit long as it includes the multiple copies of the source to
try to clear things up (only the XDR64 routines are here, as that is
the only thing I touched.)
The XDR patch that I gave out only touches the 64bit routines in the
XDR library, so here is the XDR library before my patch:
/*
* XDR 64-bit integers
*/
bool_t
xdr_int64_t(xdrs, int64_p)
register XDR *xdrs;
int64_t *int64_p;
{
int64_t x;
switch (xdrs->x_op) {
case XDR_ENCODE:
return (xdr_opaque(xdrs, (caddr_t)int64_p, sizeof(int64_t)));
case XDR_DECODE:
if (!xdr_opaque(xdrs, (caddr_t)&x, sizeof x)) {
return (FALSE);
}
*int64_p = x;
return (TRUE);
case XDR_FREE:
return (TRUE);
}
return (FALSE);
}
/*
* XDR unsigned 64-bit integers
*/
bool_t
xdr_u_int64_t(xdrs, uint64_p)
register XDR *xdrs;
u_int64_t *uint64_p;
{
u_int64_t x;
switch (xdrs->x_op) {
case XDR_ENCODE:
return (xdr_opaque(xdrs, (caddr_t)uint64_p, sizeof(u_int64_t)));
case XDR_DECODE:
if (!xdr_opaque(xdrs, (caddr_t)&x, sizeof x)) {
return (FALSE);
}
*uint64_p = x;
return (TRUE);
case XDR_FREE:
return (TRUE);
}
return (FALSE);
}
The following is from:
http://www.freebsd.org/cgi/cvsweb.cgi/basesrc/lib/libc/rpc/xdr.c?rev=1.21&cvsroot=netbsd
/*
* XDR 64-bit integers
*/
bool_t
xdr_int64_t(xdrs, llp)
XDR *xdrs;
int64_t *llp;
{
u_long ul[2];
switch (xdrs->x_op) {
case XDR_ENCODE:
ul[0] = (u_long)((u_int64_t)*llp >> 32) & 0xffffffff;
ul[1] = (u_long)((u_int64_t)*llp) & 0xffffffff;
if (XDR_PUTLONG(xdrs, (long *)&ul[0]) == FALSE)
return (FALSE);
return (XDR_PUTLONG(xdrs, (long *)&ul[1]));
case XDR_DECODE:
if (XDR_GETLONG(xdrs, (long *)&ul[0]) == FALSE)
return (FALSE);
if (XDR_GETLONG(xdrs, (long *)&ul[1]) == FALSE)
return (FALSE);
*llp = (int64_t)
(((u_int64_t)ul[0] << 32) | ((u_int64_t)ul[1]));
return (TRUE);
case XDR_FREE:
return (TRUE);
}
/* NOTREACHED */
return (FALSE);
}
/*
* XDR unsigned 64-bit integers
*/
bool_t
xdr_u_int64_t(xdrs, ullp)
XDR *xdrs;
u_int64_t *ullp;
{
u_long ul[2];
switch (xdrs->x_op) {
case XDR_ENCODE:
ul[0] = (u_long)(*ullp >> 32) & 0xffffffff;
ul[1] = (u_long)(*ullp) & 0xffffffff;
if (XDR_PUTLONG(xdrs, (long *)&ul[0]) == FALSE)
return (FALSE);
return (XDR_PUTLONG(xdrs, (long *)&ul[1]));
case XDR_DECODE:
if (XDR_GETLONG(xdrs, (long *)&ul[0]) == FALSE)
return (FALSE);
if (XDR_GETLONG(xdrs, (long *)&ul[1]) == FALSE)
return (FALSE);
*ullp = (u_int64_t)
(((u_int64_t)ul[0] << 32) | ((u_int64_t)ul[1]));
return (TRUE);
case XDR_FREE:
return (TRUE);
}
/* NOTREACHED */
return (FALSE);
}
The following is a result of:
patch <xdr.diff
/*
* XDR 64-bit integers
*/
bool_t
xdr_int64_t(xdrs, int64_p)
register XDR *xdrs;
int64_t *int64_p;
{
u_long ul[2];
switch (xdrs->x_op) {
case XDR_ENCODE:
ul[0] = (u_long)((u_int64_t)*int64_p >> 32) & 0xffffffff;
ul[1] = (u_long)((u_int64_t)*int64_p) & 0xffffffff;
if (XDR_PUTLONG(xdrs, (long *)&ul[0]) == FALSE)
return (FALSE);
return (XDR_PUTLONG(xdrs, (long *)&ul[1]));
case XDR_DECODE:
if (XDR_GETLONG(xdrs, (long *)&ul[0]) == FALSE)
return (FALSE);
if (XDR_GETLONG(xdrs, (long *)&ul[1]) == FALSE)
return (FALSE);
*int64_p = (int64_t)
(((u_int64_t)ul[0] << 32) | ((u_int64_t)ul[1]));
return (TRUE);
case XDR_FREE:
return (TRUE);
}
return (FALSE);
}
/*
* XDR unsigned 64-bit integers
*/
bool_t
xdr_u_int64_t(xdrs, uint64_p)
register XDR *xdrs;
u_int64_t *uint64_p;
{
u_long ul[2];
switch (xdrs->x_op) {
case XDR_ENCODE:
ul[0] = (u_long)(*int64_p >> 32) & 0xffffffff;
ul[1] = (u_long)(*int64_p) & 0xffffffff;
if (XDR_PUTLONG(xdrs, (long *)&ul[0]) == FALSE)
return (FALSE);
return (XDR_PUTLONG(xdrs, (long *)&ul[1]));
case XDR_DECODE:
if (XDR_GETLONG(xdrs, (long *)&ul[0]) == FALSE)
return (FALSE);
if (XDR_GETLONG(xdrs, (long *)&ul[1]) == FALSE)
return (FALSE);
*int64_p = (u_int64_t)
(((u_int64_t)ul[0] << 32) | ((u_int64_t)ul[1]));
return (TRUE);
case XDR_FREE:
return (TRUE);
}
return (FALSE);
}
So, if we save the NetBSD and the patched FreeBSD xdr.c (just the XDR64
routeines) to a file, and diff them we get:
1,2d0
<
<
7,9c5,7
< xdr_int64_t(xdrs, llp)
< XDR *xdrs;
< int64_t *llp;
---
> xdr_int64_t(xdrs, int64_p)
> register XDR *xdrs;
> int64_t *int64_p;
13a12
>
15,16c14,15
< ul[0] = (u_long)((u_int64_t)*llp >> 32) & 0xffffffff;
< ul[1] = (u_long)((u_int64_t)*llp) & 0xffffffff;
---
> ul[0] = (u_long)((u_int64_t)*int64_p >> 32) & 0xffffffff;
> ul[1] = (u_long)((u_int64_t)*int64_p) & 0xffffffff;
25,26c24,25
< *llp = (int64_t)
< (((u_int64_t)ul[0] << 32) | ((u_int64_t)ul[1]));
---
> *int64_p = (int64_t)
> (((u_int64_t)ul[0] << 32) | ((u_int64_t)ul[1]));
31d29
< /* NOTREACHED */
35d32
<
40,42c37,39
< xdr_u_int64_t(xdrs, ullp)
< XDR *xdrs;
< u_int64_t *ullp;
---
> xdr_u_int64_t(xdrs, uint64_p)
> register XDR *xdrs;
> u_int64_t *uint64_p;
46a44
>
48,49c46,47
< ul[0] = (u_long)(*ullp >> 32) & 0xffffffff;
< ul[1] = (u_long)(*ullp) & 0xffffffff;
---
> ul[0] = (u_long)(*int64_p >> 32) & 0xffffffff;
> ul[1] = (u_long)(*int64_p) & 0xffffffff;
52a51
>
58,59c57,58
< *ullp = (u_int64_t)
< (((u_int64_t)ul[0] << 32) | ((u_int64_t)ul[1]));
---
> *int64_p = (u_int64_t)
> (((u_int64_t)ul[0] << 32) | ((u_int64_t)ul[1]));
64d62
< /* NOTREACHED */
The only differences that I am seeing in the file are naming conventions.
I decided to stick with ours. The NetBSD people have the 64bit type as the
long long and the unsigned long long, where we have the int64_t and
u_int64_t. They make mention of this being non-portable, and thus in its
own secion. As we have the 'int64_t' I decided to stick with it.
I will ask the lockd devloper to do an additional set of runs
just to make absolutely, 100%, definitely, undeniably, sure.
--
David Cross | email: crossd@cs.rpi.edu
Lab Director | Rm: 308 Lally Hall
Rensselaer Polytechnic Institute, | Ph: 518.276.2860
Department of Computer Science | Fax: 518.276.4033
I speak only for myself. | WinNT:Linux::Linux:FreeBSD
Ok, cool, looking forward to seeing the commit :) Just as a sidebar, the developer re-tested the NFS locking code against the changes and everything looks 100% :) (I will be publishing the second beta shortly, just need to clean up the makefile, etc.) -- David Cross | email: crossd@cs.rpi.edu Lab Director | Rm: 308 Lally Hall Rensselaer Polytechnic Institute, | Ph: 518.276.2860 Department of Computer Science | Fax: 518.276.4033 I speak only for myself. | WinNT:Linux::Linux:FreeBSD Responsible Changed From-To: freebsd-bugs->sheldonh I'll take this one. On Thu, 06 Apr 2000 14:19:01 -0400, "David E. Cross" wrote:
> Hmm.. I feel one of us is probably doing something stupid :)
Ah, yes that would be me. :-)
I don't know how but I managed to look at the wrong functions in the
NetBSD code. Sorry about the inconvenience.
Ciao,
Sheldon.
When will those hit the CVS tree? -- David Cross | email: crossd@cs.rpi.edu Lab Director | Rm: 308 Lally Hall Rensselaer Polytechnic Institute, | Ph: 518.276.2860 Department of Computer Science | Fax: 518.276.4033 I speak only for myself. | WinNT:Linux::Linux:FreeBSD On Fri, 07 Apr 2000 19:23:16 -0400, "David E. Cross" wrote:
> When will those hit the CVS tree?
Sorry, some documentation work that I'm busy with took longer than
expected. I'll do it in about 19 hours from the time I sent this mail.
Ciao,
Sheldon.
Hi David, Sorry again for the delay. I want to test this through a ``make world'' and last night's automated build broke because of CVS conflicts. I'm making world as I type. Ciao, Sheldon. State Changed From-To: open->analyzed Committed in rev 1.10. David, I know you want this for your lockd stuff, but I'm resisting the urge to merge this onto the RELENG_4 branch immediately. Would you bug me in 1 week? Hi David, You happy with the XDR stuff being merged into RELENG_4? Have you tested it on that branch? Ciao, Sheldon. Responsible Changed From-To: sheldonh->dec David has a commit bit now. State Changed From-To: analyzed->closed dec MFC'd this in revision 1.9.2.1 of libc/xdr/xdr.c |
The XDR routines for 64bit data types do not propperly byte-swap the data. Fix: Apply the patch derived from NetBSD which is available at http://www.cs.rpi.edu/~crossd/FreeBSD/xdr.diff How-To-Repeat: Write an NFSv4 Locking implimentation. Submit it for public testing, get back many error reports.