Page MenuHomeFreeBSD

libc/xdr: remove bogus lseek(2) for xdr streams
ClosedPublic

Authored by glebius on Thu, Dec 26, 9:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jan 11, 7:58 AM
Unknown Object (File)
Sat, Jan 11, 4:23 AM
Unknown Object (File)
Sat, Jan 11, 4:04 AM
Unknown Object (File)
Fri, Jan 10, 1:56 PM
Unknown Object (File)
Fri, Jan 10, 11:53 AM
Unknown Object (File)
Fri, Jan 10, 12:56 AM
Unknown Object (File)
Mon, Jan 6, 4:22 PM
Unknown Object (File)
Wed, Jan 1, 6:08 PM
Subscribers

Details

Summary

Doing some debugging I noticed that applications using rpc(3) would often
make lseek(2) on a totally bogus file descriptor, that looks more like a
pointer. So, what happens here is that xdrrec type xdr doesn't keep a
track of how many bytes were sent/received on the stream and tries to
obtain this number via lseek(2). Then it adds/subtracts the offset in the
internal buffer from the obtained number. This code originates from the
original Sun RPC import in 1994. However, it was not a working code even
if Solaris would support lseek(2) on a socket, because it was passing not
the file descriptor, but a pointer to opaque data from upper RPC layer.
It could be that previously (before import to FreeBSD) code was correct,
but the Solaris 8 documentation says that lseek(2) on socket isn't
supported [1]. Maybe supported on older Solaris?

Anyway, this lseek(2) never worked and xdr_getpos() would always fail on
xdrrec object, until 8f55a568f69c5 in 2008 it was slightly fixed to
tolerate failure of lseek(2) and return a correct value within the small
internal buffer for XDR_ENCODE mode and a an incorrect (negative to
unsigned) result for XDR_DECODE. It seems no consumer ever calls
xdr_getpos()/xdr_setpos() on this kind of descriptor when in XDR_DECODE
mode.

So, remove this lseek(2) and preserve operation within the small buffer
only. Supposedly fix the operation for XDR_DECODE mode. Note that there
is no use and no test coverage for the XDR_DECODE.

Note that xdr(3) manual page already documents limitations for
xdr_getpos() and xdr_setpos() for the stream type objects.

[1] https://docs.oracle.com/cd/E19109-01/tsolaris8/835-8003/6ruu1b0or/index.html

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

After staring at this for a while, I think it's right.

Note that there is no use and no test coverage for the XDR_DECODE.

Isn't it used by svc_vc.c and clnt_vc.c? Any consumers of those interfaces would use this, it looks like, but there is no use of XDR_GET/SETPOS there that is affected by the bug, I think.

lib/libc/xdr/xdr_rec.c
332

ptrdiff_t would be a bit more natural here.

342

I don't see any reason to remove the break?

This revision is now accepted and ready to land.Fri, Jan 3, 7:11 PM
  • use ptrdiff_t
  • put back break
This revision now requires review to proceed.Fri, Jan 3, 7:20 PM

Isn't it used by svc_vc.c and clnt_vc.c? Any consumers of those interfaces would use this, it looks like, but there is no use of XDR_GET/SETPOS there that is affected by the bug, I think.

Right. When they use xdrrec in the XDR_DECODE mode they never do xdr_getpos/setpos.

This revision is now accepted and ready to land.Fri, Jan 3, 7:22 PM

libtirpc did something similar: https://github.com/alisw/libtirpc/commit/1e786fc401ff625fdcec3e0bdc495125feb0a070 . NetBSD and OpenBSD still contain the lseek.