* [Possible Bug] Use of write on size-limited platforms @ 2020-06-08 21:41 Randall S. Becker 2020-06-15 21:59 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Randall S. Becker @ 2020-06-08 21:41 UTC (permalink / raw) To: git 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Possible Bug] Use of write on size-limited platforms 2020-06-08 21:41 [Possible Bug] Use of write on size-limited platforms Randall S. Becker @ 2020-06-15 21:59 ` Jeff King 2020-06-15 22:38 ` Randall S. Becker 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2020-06-15 21:59 UTC (permalink / raw) To: Randall S. Becker; +Cc: git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [Possible Bug] Use of write on size-limited platforms 2020-06-15 21:59 ` Jeff King @ 2020-06-15 22:38 ` Randall S. Becker 2020-06-16 8:02 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Randall S. Becker @ 2020-06-15 22:38 UTC (permalink / raw) To: 'Jeff King'; +Cc: git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Possible Bug] Use of write on size-limited platforms 2020-06-15 22:38 ` Randall S. Becker @ 2020-06-16 8:02 ` Jeff King 2020-06-19 15:05 ` Randall S. Becker 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2020-06-16 8:02 UTC (permalink / raw) To: Randall S. Becker; +Cc: git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [Possible Bug] Use of write on size-limited platforms 2020-06-16 8:02 ` Jeff King @ 2020-06-19 15:05 ` Randall S. Becker 2020-06-19 19:35 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Randall S. Becker @ 2020-06-19 15:05 UTC (permalink / raw) To: 'Jeff King'; +Cc: git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Possible Bug] Use of write on size-limited platforms 2020-06-19 15:05 ` Randall S. Becker @ 2020-06-19 19:35 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2020-06-19 19:35 UTC (permalink / raw) To: Randall S. Becker; +Cc: 'Jeff King', git "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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-19 19:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-08 21:41 [Possible Bug] Use of write on size-limited platforms Randall S. Becker 2020-06-15 21:59 ` Jeff King 2020-06-15 22:38 ` Randall S. Becker 2020-06-16 8:02 ` Jeff King 2020-06-19 15:05 ` Randall S. Becker 2020-06-19 19:35 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).