Re: and _PC_PIPE_BUF..

[Robert Millan]

tag 184345 - fixed
tag 184345 - patch
tag 184345 - pending
severity 184345 normal
retitle 184345 undefined PIPE_BUF bug wasn't fixed properly
thanks

as we've been already discussing in help-hurd mailing list, the
patch i sent had the problem that the situation when fpathconf
returns -1 was not considered. this could lead read() to take
a negative SIZE argument as unsigned, thus reading a quantity
near 2^32 bytes (or untill EOF). discussion follows...

On Thu, Apr 24, 2003 at 10:06:05AM +0200, Niels Möller wrote:
> 
> > i'd better show an example, this is fixed code from gs. does it seem
> > correct to you? (considering the file descriptor is expected to be
> > a socket, if pipe_buf is -1 then something weird happened)
> 
> Robert Millan <zeratul2@wanadoo.es> writes:
> > /* Get packet from the server. */
> > private int hpijs_get_pk(PK * pk, int fd)
> > {
> > #ifdef PIPE_BUF
> >    return read(fd, pk, PIPE_BUF - 1);
> > #else
> >    long int pipe_buf = fpathconf(fd, _PC_PIPE_BUF);
> >    if (pipe_buf == -1)
> >      /* no pipe_buf, so we fail */
> >      return -1;
> >    else
> >      return read(fd, pk, pipe_buf - 1);
> > #endif
> > }
> 
> To me, the code looks utterly broken. What is the allocated size for
> pk? If PIPE_BUF is larger than that size, and the other end of the
> pipe writes lots of data, you will get a buffer overflow, and
> depending on context, that can turn out to be a security hole big
> enough to drive a truck through it. The size argument to read must
> *never* be larger than the size of the buffer passed to read.
> 
> Furthermore, I'm not sure what the code tries to use PIPE_BUF for. The
> only use of PIPE_BUF I have seen is when *writing* to a pipe, where
> you have a single process reading the pipe and multiple processes
> writing to it. Using PIPE_BUF when reading sounds like something that
> might make sense if you have a single writer and multiple readers on a
> pipe. Does gs do that? Such pipe usage seems pretty obscure to me.
> 
> I would have expected some code like
> 
>   return read(fd, pk, sizeof(*pk))
> 
> or, if it really depends on PIPE_BUF semantics,
> 
>   long int pipe_buf;
> #if PIPE_BUF
>   pipe_buf = PIPE_BUF;
> #else
>   pipe_buf = fpathconf(fd, _PC_PIPE_BUF);
> #endif
>   if (pipe_buf < sizeof(*pk))
>     /* pipe_buf too small (or not existing), so we fail */
>     return -1;
>   else
>     return read(fd, pk, sizeof(*pk))
> 
> And at last, the "-1" part of "PIPE_BUF - 1" also seems strange. The
> special PIPE_BUF semantics happen for request of size up to *or equal*
> to PIPE_BUF. Quoting the glibc manual:
> 
> :    Reading or writing pipe data is "atomic" if the size of data written
> : is not greater than `PIPE_BUF'.  This means that the data transfer
>      ^^^^^^^^^^^
> : seems to be an instantaneous unit, in that nothing else in the system
> : can observe a state in which it is partially complete.  Atomic I/O may
> : not begin right away (it may need to wait for buffer space or for data),
> : but once it does begin it finishes immediately.
> 
> Sorry if this isn't of much help, but I don't understand how that code
> is supposed to work.
> 
> /Niels
> 

-- 
Robert Millan

make: *** No rule to make target `war'.  Stop.

Another world is possible - Just say no to genocide