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

> 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.

> 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().

> > 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?

> 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.

> 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.