[Possible Bug] Use of write on size-limited platforms

[Possible Bug] Use of write on size-limited platforms

[Randall S. Becker]

I just wanted to check the following calls to make sure that it does not
fwrite or write should be xread/xwrite or are guaranteed not to exceed
MAX_IO_SIZE:

strbuf.c: strbuf_write, strbuf_write_fd, the size is not specified.

The other uses of read/write appear to be safe.

Thanks,
Randall

-- Brief whoami:
 NonStop developer since approximately 211288444200000000
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [Possible Bug] Use of write on size-limited platforms

[Jeff King]

On Mon, Jun 08, 2020 at 05:41:34PM -0400, Randall S. Becker wrote:

> I just wanted to check the following calls to make sure that it does not
> fwrite or write should be xread/xwrite or are guaranteed not to exceed
> MAX_IO_SIZE:
> 
> strbuf.c: strbuf_write, strbuf_write_fd, the size is not specified.
> 
> The other uses of read/write appear to be safe.

strbuf_write() is using fwrite(), and we don't enforce MAX_IO_SIZE with
stdio anywhere else. And I'd expect in general that if there are any
platform limitations, the system libc would choose a sane value anyway.
So that one is probably fine.

I think strbuf_write_fd() is wrong to use a raw write(), but for several
reasons:

 - it won't enforce MAX_IO_SIZE, as you note

 - it won't handle EINTR, etc; callers need to be prepared to restart
   such a write

 - it won't handle a partial write by looping until all output is sent

For the latter two, there are cases where some callers want the
flexibility to stop when seeing a signal or a partial write. But I don't
think that makes any sense for strbuf_write_fd(). If I pass in a strbuf
with 8kb of data and I get a return value that indicates we only wrote
4kb, what do I do next? I certainly can't call strbuf_write_fd() again,
since it would write from the beginning of the strbuf again. I'd have to
call xwrite() myself after that. At which point I may as well have done
so for the first call. :)

So I think this really ought to be using write_in_full(). There's only
one caller, and I think it would be improved by the switch. Do you want
to write a patch?

You could make an argument that the fwrite() version ought to also loop,
since it's possible to get a partial write there, too. But we don't do
that in general. I suspect in practice most stdio implementations will
keep writing until they see an error, and most callers don't bother
checking stdio errors at all, or use ferror().

-Peff

RE: [Possible Bug] Use of write on size-limited platforms

[Randall S. Becker]

On June 15, 2020 6:00 PM, Jeff King wrote:
> On Mon, Jun 08, 2020 at 05:41:34PM -0400, Randall S. Becker wrote:
> > I just wanted to check the following calls to make sure that it does
> > not fwrite or write should be xread/xwrite or are guaranteed not to
> > exceed
> > MAX_IO_SIZE:
> >
> > strbuf.c: strbuf_write, strbuf_write_fd, the size is not specified.
> >
> > The other uses of read/write appear to be safe.
> 
> strbuf_write() is using fwrite(), and we don't enforce MAX_IO_SIZE with stdio
> anywhere else. And I'd expect in general that if there are any platform
> limitations, the system libc would choose a sane value anyway.
> So that one is probably fine.
> 
> I think strbuf_write_fd() is wrong to use a raw write(), but for several
> reasons:
> 
>  - it won't enforce MAX_IO_SIZE, as you note
> 
>  - it won't handle EINTR, etc; callers need to be prepared to restart
>    such a write
> 
>  - it won't handle a partial write by looping until all output is sent
> 
> For the latter two, there are cases where some callers want the flexibility to
> stop when seeing a signal or a partial write. But I don't think that makes any
> sense for strbuf_write_fd(). If I pass in a strbuf with 8kb of data and I get a
> return value that indicates we only wrote 4kb, what do I do next? I certainly
> can't call strbuf_write_fd() again, since it would write from the beginning of
> the strbuf again. I'd have to call xwrite() myself after that. At which point I
> may as well have done so for the first call. :)
> 
> So I think this really ought to be using write_in_full(). There's only one caller,
> and I think it would be improved by the switch. Do you want to write a
> patch?
> 
> You could make an argument that the fwrite() version ought to also loop,
> since it's possible to get a partial write there, too. But we don't do that in
> general. I suspect in practice most stdio implementations will keep writing
> until they see an error, and most callers don't bother checking stdio errors at
> all, or use ferror().

I'll give the patch a go. It is very simple. Would you suggest removing the strbuf_write_fd() as part of this patch since it only impacts bugreport.c?

Regards,
Randall

Re: [Possible Bug] Use of write on size-limited platforms

[Jeff King]

On Mon, Jun 15, 2020 at 06:38:34PM -0400, Randall S. Becker wrote:

> > So I think this really ought to be using write_in_full(). There's only one caller,
> > and I think it would be improved by the switch. Do you want to write a
> > patch?
> > 
> > You could make an argument that the fwrite() version ought to also loop,
> > since it's possible to get a partial write there, too. But we don't do that in
> > general. I suspect in practice most stdio implementations will keep writing
> > until they see an error, and most callers don't bother checking stdio errors at
> > all, or use ferror().
> 
> I'll give the patch a go. It is very simple. Would you suggest
> removing the strbuf_write_fd() as part of this patch since it only
> impacts bugreport.c?

I could go either way on that. IMHO it isn't offering much over a bare
write_in_full() call. The "don't call write() if there are 0 bytes"
logic is part of write_in_full() already.

-Peff

RE: [Possible Bug] Use of write on size-limited platforms

[Randall S. Becker]

On June 16, 2020 4:02 AM, Peff wrote:
> To: Randall S. Becker <rsbecker@nexbridge.com>
> Cc: git@vger.kernel.org
> Subject: Re: [Possible Bug] Use of write on size-limited platforms
> 
> On Mon, Jun 15, 2020 at 06:38:34PM -0400, Randall S. Becker wrote:
> 
> > > So I think this really ought to be using write_in_full(). There's
> > > only one caller, and I think it would be improved by the switch. Do
> > > you want to write a patch?
> > >
> > > You could make an argument that the fwrite() version ought to also
> > > loop, since it's possible to get a partial write there, too. But we
> > > don't do that in general. I suspect in practice most stdio
> > > implementations will keep writing until they see an error, and most
> > > callers don't bother checking stdio errors at all, or use ferror().
> >
> > I'll give the patch a go. It is very simple. Would you suggest
> > removing the strbuf_write_fd() as part of this patch since it only
> > impacts bugreport.c?
> 
> I could go either way on that. IMHO it isn't offering much over a bare
> write_in_full() call. The "don't call write() if there are 0 bytes"
> logic is part of write_in_full() already.

Patch delivered.

Regards,
Randall

Re: [Possible Bug] Use of write on size-limited platforms

[Junio C Hamano]

"Randall S. Becker" <rsbecker@nexbridge.com> writes:

> On June 16, 2020 4:02 AM, Peff wrote:
>> To: Randall S. Becker <rsbecker@nexbridge.com>
>> Cc: git@vger.kernel.org
>> Subject: Re: [Possible Bug] Use of write on size-limited platforms
>> 
>> On Mon, Jun 15, 2020 at 06:38:34PM -0400, Randall S. Becker wrote:
>> 
>> > > So I think this really ought to be using write_in_full(). There's
>> > > only one caller, and I think it would be improved by the switch. Do
>> > > you want to write a patch?
>> > >
>> > > You could make an argument that the fwrite() version ought to also
>> > > loop, since it's possible to get a partial write there, too. But we
>> > > don't do that in general. I suspect in practice most stdio
>> > > implementations will keep writing until they see an error, and most
>> > > callers don't bother checking stdio errors at all, or use ferror().
>> >
>> > I'll give the patch a go. It is very simple. Would you suggest
>> > removing the strbuf_write_fd() as part of this patch since it only
>> > impacts bugreport.c?
>> 
>> I could go either way on that. IMHO it isn't offering much over a bare
>> write_in_full() call. The "don't call write() if there are 0 bytes"
>> logic is part of write_in_full() already.
>
> Patch delivered.

I think these go in the right direction and would be good with minor
fixes.  I do agree with the other reviewer that dumping all the
contents of the buffer to die() may not be what we want (do we even
need to see the contents at all?).  Even though I do not mind too
much about the unneeded {braces}, I'd prefer to see it fixed if we
are rerolling the patch for other reason anyway.  And I think 2 and
3 can and should be a single patch---if we are removing a function
that is no longer used, we should remove both its decl and defn at
the same time.

Thanks.