{"thread":{"id":"53638","subject":"[Possible Bug] Use of write on size-limited platforms","startedAt":"2020-06-08T21:41:47Z","lastAt":"2020-06-19T19:35:27Z","messageCount":6,"participants":["Randall S. Becker","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"399321","messageId":"015c01d63ddd$97d65080$c782f180$@nexbridge.com","threadId":"53638","inReplyTo":null,"subject":"[Possible Bug] Use of write on size-limited platforms","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2020-06-08T21:41:34Z","receivedAt":"2020-06-08T21:41:47Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"I just wanted to check the following calls to make sure that it does not\nfwrite or write should be xread/xwrite or are guaranteed not to exceed\nMAX_IO_SIZE:\n\nstrbuf.c: strbuf_write, strbuf_write_fd, the size is not specified.\n\nThe other uses of read/write appear to be safe.\n\nThanks,\nRandall\n\n-- Brief whoami:\n NonStop developer since approximately 211288444200000000\n UNIX developer since approximately 421664400\n-- In my real life, I talk too much.\n\n\n"},{"id":"399828","messageId":"20200615215937.GA636737@coredump.intra.peff.net","threadId":"53638","inReplyTo":"015c01d63ddd$97d65080$c782f180$@nexbridge.com","subject":"Re: [Possible Bug] Use of write on size-limited platforms","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-06-15T21:59:37Z","receivedAt":"2020-06-15T21:59:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 08, 2020 at 05:41:34PM -0400, Randall S. Becker wrote:\n\n> I just wanted to check the following calls to make sure that it does not\n> fwrite or write should be xread/xwrite or are guaranteed not to exceed\n> MAX_IO_SIZE:\n> \n> strbuf.c: strbuf_write, strbuf_write_fd, the size is not specified.\n> \n> The other uses of read/write appear to be safe.\n\nstrbuf_write() is using fwrite(), and we don't enforce MAX_IO_SIZE with\nstdio anywhere else. And I'd expect in general that if there are any\nplatform limitations, the system libc would choose a sane value anyway.\nSo that one is probably fine.\n\nI think strbuf_write_fd() is wrong to use a raw write(), but for several\nreasons:\n\n - it won't enforce MAX_IO_SIZE, as you note\n\n - it won't handle EINTR, etc; callers need to be prepared to restart\n   such a write\n\n - it won't handle a partial write by looping until all output is sent\n\nFor the latter two, there are cases where some callers want the\nflexibility to stop when seeing a signal or a partial write. But I don't\nthink that makes any sense for strbuf_write_fd(). If I pass in a strbuf\nwith 8kb of data and I get a return value that indicates we only wrote\n4kb, what do I do next? I certainly can't call strbuf_write_fd() again,\nsince it would write from the beginning of the strbuf again. I'd have to\ncall xwrite() myself after that. At which point I may as well have done\nso for the first call. :)\n\nSo I think this really ought to be using write_in_full(). There's only\none caller, and I think it would be improved by the switch. Do you want\nto write a patch?\n\nYou could make an argument that the fwrite() version ought to also loop,\nsince it's possible to get a partial write there, too. But we don't do\nthat in general. I suspect in practice most stdio implementations will\nkeep writing until they see an error, and most callers don't bother\nchecking stdio errors at all, or use ferror().\n\n-Peff\n"},{"id":"399830","messageId":"002801d64365$b6fdc2d0$24f94870$@nexbridge.com","threadId":"53638","inReplyTo":"20200615215937.GA636737@coredump.intra.peff.net","subject":"RE: [Possible Bug] Use of write on size-limited platforms","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2020-06-15T22:38:34Z","receivedAt":"2020-06-15T22:38:46Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On June 15, 2020 6:00 PM, Jeff King wrote:\n> On Mon, Jun 08, 2020 at 05:41:34PM -0400, Randall S. Becker wrote:\n> > I just wanted to check the following calls to make sure that it does\n> > not fwrite or write should be xread/xwrite or are guaranteed not to\n> > exceed\n> > MAX_IO_SIZE:\n> >\n> > strbuf.c: strbuf_write, strbuf_write_fd, the size is not specified.\n> >\n> > The other uses of read/write appear to be safe.\n> \n> strbuf_write() is using fwrite(), and we don't enforce MAX_IO_SIZE with stdio\n> anywhere else. And I'd expect in general that if there are any platform\n> limitations, the system libc would choose a sane value anyway.\n> So that one is probably fine.\n> \n> I think strbuf_write_fd() is wrong to use a raw write(), but for several\n> reasons:\n> \n>  - it won't enforce MAX_IO_SIZE, as you note\n> \n>  - it won't handle EINTR, etc; callers need to be prepared to restart\n>    such a write\n> \n>  - it won't handle a partial write by looping until all output is sent\n> \n> For the latter two, there are cases where some callers want the flexibility to\n> stop when seeing a signal or a partial write. But I don't think that makes any\n> sense for strbuf_write_fd(). If I pass in a strbuf with 8kb of data and I get a\n> return value that indicates we only wrote 4kb, what do I do next? I certainly\n> can't call strbuf_write_fd() again, since it would write from the beginning of\n> the strbuf again. I'd have to call xwrite() myself after that. At which point I\n> may as well have done so for the first call. :)\n> \n> So I think this really ought to be using write_in_full(). There's only one caller,\n> and I think it would be improved by the switch. Do you want to write a\n> patch?\n> \n> You could make an argument that the fwrite() version ought to also loop,\n> since it's possible to get a partial write there, too. But we don't do that in\n> general. I suspect in practice most stdio implementations will keep writing\n> until they see an error, and most callers don't bother checking stdio errors at\n> all, or use ferror().\n\nI'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?\n\nRegards,\nRandall\n\n"},{"id":"399853","messageId":"20200616080200.GA650578@coredump.intra.peff.net","threadId":"53638","inReplyTo":"002801d64365$b6fdc2d0$24f94870$@nexbridge.com","subject":"Re: [Possible Bug] Use of write on size-limited platforms","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-06-16T08:02:00Z","receivedAt":"2020-06-16T08:02:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 15, 2020 at 06:38:34PM -0400, Randall S. Becker wrote:\n\n> > So I think this really ought to be using write_in_full(). There's only one caller,\n> > and I think it would be improved by the switch. Do you want to write a\n> > patch?\n> > \n> > You could make an argument that the fwrite() version ought to also loop,\n> > since it's possible to get a partial write there, too. But we don't do that in\n> > general. I suspect in practice most stdio implementations will keep writing\n> > until they see an error, and most callers don't bother checking stdio errors at\n> > all, or use ferror().\n> \n> I'll give the patch a go. It is very simple. Would you suggest\n> removing the strbuf_write_fd() as part of this patch since it only\n> impacts bugreport.c?\n\nI could go either way on that. IMHO it isn't offering much over a bare\nwrite_in_full() call. The \"don't call write() if there are 0 bytes\"\nlogic is part of write_in_full() already.\n\n-Peff\n"},{"id":"400136","messageId":"028c01d6464b$235346c0$69f9d440$@nexbridge.com","threadId":"53638","inReplyTo":"20200616080200.GA650578@coredump.intra.peff.net","subject":"RE: [Possible Bug] Use of write on size-limited platforms","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2020-06-19T15:05:52Z","receivedAt":"2020-06-19T16:11:13Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On June 16, 2020 4:02 AM, Peff wrote:\n> To: Randall S. Becker <rsbecker@nexbridge.com>\n> Cc: git@vger.kernel.org\n> Subject: Re: [Possible Bug] Use of write on size-limited platforms\n> \n> On Mon, Jun 15, 2020 at 06:38:34PM -0400, Randall S. Becker wrote:\n> \n> > > So I think this really ought to be using write_in_full(). There's\n> > > only one caller, and I think it would be improved by the switch. Do\n> > > you want to write a patch?\n> > >\n> > > You could make an argument that the fwrite() version ought to also\n> > > loop, since it's possible to get a partial write there, too. But we\n> > > don't do that in general. I suspect in practice most stdio\n> > > implementations will keep writing until they see an error, and most\n> > > callers don't bother checking stdio errors at all, or use ferror().\n> >\n> > I'll give the patch a go. It is very simple. Would you suggest\n> > removing the strbuf_write_fd() as part of this patch since it only\n> > impacts bugreport.c?\n> \n> I could go either way on that. IMHO it isn't offering much over a bare\n> write_in_full() call. The \"don't call write() if there are 0 bytes\"\n> logic is part of write_in_full() already.\n\nPatch delivered.\n\nRegards,\nRandall\n\n"},{"id":"400213","messageId":"xmqq4kr6hmrs.fsf@gitster.c.googlers.com","threadId":"53638","inReplyTo":"028c01d6464b$235346c0$69f9d440$@nexbridge.com","subject":"Re: [Possible Bug] Use of write on size-limited platforms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-19T19:35:19Z","receivedAt":"2020-06-19T19:35:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" <rsbecker@nexbridge.com> writes:\n\n> On June 16, 2020 4:02 AM, Peff wrote:\n>> To: Randall S. Becker <rsbecker@nexbridge.com>\n>> Cc: git@vger.kernel.org\n>> Subject: Re: [Possible Bug] Use of write on size-limited platforms\n>> \n>> On Mon, Jun 15, 2020 at 06:38:34PM -0400, Randall S. Becker wrote:\n>> \n>> > > So I think this really ought to be using write_in_full(). There's\n>> > > only one caller, and I think it would be improved by the switch. Do\n>> > > you want to write a patch?\n>> > >\n>> > > You could make an argument that the fwrite() version ought to also\n>> > > loop, since it's possible to get a partial write there, too. But we\n>> > > don't do that in general. I suspect in practice most stdio\n>> > > implementations will keep writing until they see an error, and most\n>> > > callers don't bother checking stdio errors at all, or use ferror().\n>> >\n>> > I'll give the patch a go. It is very simple. Would you suggest\n>> > removing the strbuf_write_fd() as part of this patch since it only\n>> > impacts bugreport.c?\n>> \n>> I could go either way on that. IMHO it isn't offering much over a bare\n>> write_in_full() call. The \"don't call write() if there are 0 bytes\"\n>> logic is part of write_in_full() already.\n>\n> Patch delivered.\n\nI think these go in the right direction and would be good with minor\nfixes.  I do agree with the other reviewer that dumping all the\ncontents of the buffer to die() may not be what we want (do we even\nneed to see the contents at all?).  Even though I do not mind too\nmuch about the unneeded {braces}, I'd prefer to see it fixed if we\nare rerolling the patch for other reason anyway.  And I think 2 and\n3 can and should be a single patch---if we are removing a function\nthat is no longer used, we should remove both its decl and defn at\nthe same time.\n\nThanks.\n"}]}