On Monday, February 26, 2024 11:00 AM, Phillip Wood wrote:
>On 26/02/2024 15:32, Phillip Wood wrote:
>> Hi Randal
>>
>> [cc'ing Patrick for the reftable writer]
>>
>> On 25/02/2024 20:36, rsbecker@xxxxxxxxxxxxx wrote:
>>> On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote:
>>>> On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@xxxxxxxxxxxxx wrote:
>>>>> On Sunday, February 25, 2024 1:45 PM, I wrote:
>>>>>> To: git@xxxxxxxxxxxxxxx
>>>> But I think that this should be used:
>>>> write_in_full()
>>>
>>> My mailer autocorrected, yes, xwrite. write_in_full() would be safe,
>>> although a bit redundant since xwrite() does similar things and is
>>> used by write_in_full().
>>
>> Note that unlike write_in_full(), xwrite() does not guarantee to write
>> the whole buffer passed to it. In general unless a caller is writing a
>> single byte or writing less than PIPE_BUF bytes to a pipe it should
>> use write_in_full().
>>
>>> The question is which call is bad? The cruft stuff is relatively new
>>> and I don't know the code.
>
>I should have been clearer that I do not think any of these calls are the likely
>problem for the cruft pack code. I do think the reftable writers are worth looking at
>though for git in general.
>
>For the cruft pack problem you might want to look for suspect xwrite() calls where
>the caller does not handle a short write correctly for example under builtin/ we have
>
>builtin/index-pack.c: err = xwrite(1, input_buffer +
>input_offset, input_len);
>builtin/receive-pack.c: xwrite(2, msg, sz);
>builtin/repack.c: xwrite(cmd->in, oid_to_hex(oid),
>the_hash_algo->hexsz);
>builtin/repack.c: xwrite(cmd->in, "\n", 1);
>builtin/unpack-objects.c: int ret = xwrite(1, buffer +
>offset, len);
>
>Best Wishes
>
>Phillip
>
>>>>> reftable/writer.c: int n = w->write(w->write_arg,
>>>>> zeroed,
>>>>> w->pending_padding);
>>>>> reftable/writer.c: n = w->write(w->write_arg, data, len);
>>
>> Neither of these appear to check for short writes and
>> reftable_fd_write() is a thin wrapper around write(). Maybe
>> reftable_fd_write() should be using write_in_full()?
>>
>>>>> run-command.c: len = write(io->fd, io->u.out.buf,
>>
>> This call to write() looks correct as it is in the io pump loop.
>>
>>>>> t/helper/test-path-utils.c: if (write(1,
>>>>> buffer,
>>> count)
>>>>> < 0) >>> t/helper/test-windows-named-pipe.c: write(1,
>>>>> buf, nbr);
>>>>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
>>
>> In principle these all look like they are prone to short writes.
>>
>>>>> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf,
>>>>> buf_line->len);
>>
>> This caller explicitly says it prefers short writes over retrying
Replacing xwrite with write_in_full the above worked correctly. Do you want it or should I write this up?
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a3a37bd215..f80b8d101a 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
* the last part of the input buffer to stdout.
*/
while (input_len) {
- err = xwrite(1, input_buffer + input_offset, input_len);
+ err = write_in_full(1, input_buffer + input_offset, input_len);
if (err <= 0)
break;
input_len -= err;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index db65607485..4277c63d08 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -456,7 +456,7 @@ static void report_message(const char *prefix, const char *err, va_list params)
if (use_sideband)
send_sideband(1, 2, msg, sz, use_sideband);
else
- xwrite(2, msg, sz);
+ write_in_full(2, msg, sz);
}
__attribute__((format (printf, 1, 2)))
diff --git a/builtin/repack.c b/builtin/repack.c
index ede36328a3..4916870992 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -314,8 +314,8 @@ static int write_oid(const struct object_id *oid,
die(_("could not start pack-objects to repack promisor objects"));
}
- xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
- xwrite(cmd->in, "\n", 1);
+ write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
+ write_in_full(cmd->in, "\n", 1);
return 0;
}
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index e0a701f2b3..6935c4574e 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
/* Write the last part of the buffer to stdout */
while (len) {
- int ret = xwrite(1, buffer + offset, len);
+ int ret = write_in_full(1, buffer + offset, len);
if (ret <= 0)
break;
len -= ret;
