@matheusbsbs / matheus-bagliano / commits / 06f46f237a

avoid "write_in_full(fd, buf, len) != len" pattern

The return value of write_in_full() is either "-1", or the requested number of bytes[1]. If we make a partial write before seeing an error, we still return -1, not a partial value. This goes back to f6aa66cb95 (write_in_full: really write in full or return error on disk full., 2007-01-11). So checking anything except "was the return value negative" is pointless. And there are a couple of reasons not to do so: 1. It can do a funny signed/unsigned comparison. If your "len" is signed (e.g., a size_t) then the compiler will promote the "-1" to its unsigned variant. This works out for "!= len" (unless you really were trying to write the maximum size_t bytes), but is a bug if you check "< len" (an example of which was fixed recently in config.c). We should avoid promoting the mental model that you need to check the length at all, so that new sites are not tempted to copy us. 2. Checking for a negative value is shorter to type, especially when the length is an expression. 3. Linus says so. In d34cf19b89 (Clean up write_in_full() users, 2007-01-11), right after the write_in_full() semantics were changed, he wrote: I really wish every "write_in_full()" user would just check against "<0" now, but this fixes the nasty and stupid ones. Appeals to authority aside, this makes it clear that writing it this way does not have an intentional benefit. It's a historical curiosity that we never bothered to clean up (and which was undoubtedly cargo-culted into new sites). So let's convert these obviously-correct cases (this includes write_str_in_full(), which is just a wrapper for write_in_full()). [1] A careful reader may notice there is one way that write_in_full() can return a different value. If we ask write() to write N bytes and get a return value that is _larger_ than N, we could return a larger total. But besides the fact that this would imply a totally broken version of write(), it would already invoke undefined behavior. Our internal remaining counter is an unsigned size_t, which means that subtracting too many byte will wrap it around to a very large number. So we'll instantly begin reading off the end of the buffer, trying to write gigabytes (or petabytes) of data. Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Sep 13, 2017 at 13:16 UTC 06f46f237afa823c0a2775e60ed8fbd80e7c751f

16 files changed +26 -27

builtin/receive-pack.c

+1 -1

@@ -742,7 +742,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,

742 size_t n;

743 if (feed(feed_state, &buf, &n))

744 break;

745 - if (write_in_full(proc.in, buf, n) != n)

745 + if (write_in_full(proc.in, buf, n) < 0)

746 break;

747 }

748 close(proc.in);

builtin/rerere.c

+1 -1

@@ -18,7 +18,7 @@ static int outf(void *dummy, mmbuffer_t *ptr, int nbuf)

18 {

19 int i;

20 for (i = 0; i < nbuf; i++)

21 - if (write_in_full(1, ptr[i].ptr, ptr[i].size) != ptr[i].size)

21 + if (write_in_full(1, ptr[i].ptr, ptr[i].size) < 0)

22 return -1;

23 return 0;

24 }

builtin/unpack-file.c

+1 -1

@@ -15,7 +15,7 @@ static char *create_temp_file(unsigned char *sha1)

16 xsnprintf(path, sizeof(path), ".merge_file_XXXXXX");

17 fd = xmkstemp(path);

18 - if (write_in_full(fd, buf, size) != size)

18 + if (write_in_full(fd, buf, size) < 0)

19 die_errno("unable to write temp-file");

20 close(fd);

21 return path;

config.c

+2 -2

@@ -2565,7 +2565,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,

2565 copy_end - copy_begin) < 0)

2566 goto write_err_out;

2567 if (new_line &&

2568 - write_str_in_full(fd, "\n") != 1)

2568 + write_str_in_full(fd, "\n") < 0)

2569 goto write_err_out;

2570 }

2571 copy_begin = store.offset[i];

@@ -2796,7 +2796,7 @@ int git_config_rename_section_in_file(const char *config_filename,

2796 if (remove)

2797 continue;

2798 length = strlen(output);

2799 - if (write_in_full(out_fd, output, length) != length) {

2799 + if (write_in_full(out_fd, output, length) < 0) {

2800 ret = write_error(get_lock_file_path(lock));

2801 goto out;

2802 }

diff.c

+1 -1

@@ -2975,7 +2975,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,

2975 blob = buf.buf;

2976 size = buf.len;

2977 }

2978 - if (write_in_full(fd, blob, size) != size)

2978 + if (write_in_full(fd, blob, size) < 0)

2979 die_errno("unable to write temp-file");

2980 close_tempfile(&temp->tempfile);

2981 temp->name = get_tempfile_path(&temp->tempfile);

fast-import.c

+1 -1

@@ -2951,7 +2951,7 @@ static void parse_reset_branch(const char *arg)

2951

2952 static void cat_blob_write(const char *buf, unsigned long size)

2953 {

2954 - if (write_in_full(cat_blob_fd, buf, size) != size)

2954 + if (write_in_full(cat_blob_fd, buf, size) < 0)

2955 die_errno("Write to frontend failed");

2956 }

2957

http-backend.c

+2 -2

@@ -357,7 +357,7 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)

357 die("zlib error inflating request, result %d", ret);

358

359 n = stream.total_out - cnt;

360 - if (write_in_full(out, out_buf, n) != n)

360 + if (write_in_full(out, out_buf, n) < 0)

361 die("%s aborted reading request", prog_name);

362 cnt += n;

363

@@ -378,7 +378,7 @@ static void copy_request(const char *prog_name, int out)

378 ssize_t n = read_request(0, &buf);

379 if (n < 0)

380 die_errno("error reading request body");

381 - if (write_in_full(out, buf, n) != n)

381 + if (write_in_full(out, buf, n) < 0)

382 die("%s aborted reading request", prog_name);

383 close(out);

384 free(buf);

ll-merge.c

+1 -1

@@ -154,7 +154,7 @@ static void create_temp(mmfile_t *src, char *path, size_t len)

154

155 xsnprintf(path, len, ".merge_file_XXXXXX");

156 fd = xmkstemp(path);

157 - if (write_in_full(fd, src->ptr, src->size) != src->size)

157 + if (write_in_full(fd, src->ptr, src->size) < 0)

158 die_errno("unable to write temp-file");

159 close(fd);

160 }

read-cache.c

+3 -3

@@ -1921,7 +1921,7 @@ static int ce_write_flush(git_SHA_CTX *context, int fd)

1921 unsigned int buffered = write_buffer_len;

1922 if (buffered) {

1923 git_SHA1_Update(context, write_buffer, buffered);

1924 - if (write_in_full(fd, write_buffer, buffered) != buffered)

1924 + if (write_in_full(fd, write_buffer, buffered) < 0)

1925 return -1;

1926 write_buffer_len = 0;

1927 }

@@ -1970,7 +1970,7 @@ static int ce_flush(git_SHA_CTX *context, int fd, unsigned char *sha1)

1970

1971 /* Flush first if not enough space for SHA1 signature */

1972 if (left + 20 > WRITE_BUFFER_SIZE) {

1973 - if (write_in_full(fd, write_buffer, left) != left)

1973 + if (write_in_full(fd, write_buffer, left) < 0)

1974 return -1;

1975 left = 0;

1976 }

@@ -1979,7 +1979,7 @@ static int ce_flush(git_SHA_CTX *context, int fd, unsigned char *sha1)

1979 git_SHA1_Final(write_buffer + left, context);

1980 hashcpy(sha1, write_buffer + left);

1981 left += 20;

1982 - return (write_in_full(fd, write_buffer, left) != left) ? -1 : 0;

1982 + return (write_in_full(fd, write_buffer, left) < 0) ? -1 : 0;

1983 }

1984

1985 static void ce_smudge_racily_clean_entry(struct cache_entry *ce)

refs.c

+1 -1

@@ -592,7 +592,7 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,

592 }

593 }

594

595 - if (write_in_full(fd, buf.buf, buf.len) != buf.len) {

595 + if (write_in_full(fd, buf.buf, buf.len) < 0) {

596 strbuf_addf(err, "could not write to '%s'", filename);

597 rollback_lock_file(&lock);

598 goto done;

refs/files-backend.c

+4 -4

@@ -2118,8 +2118,8 @@ static int write_ref_to_lockfile(struct ref_lock *lock,

2118 return -1;

2119 }

2120 fd = get_lock_file_fd(lock->lk);

2121 - if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||

2122 - write_in_full(fd, &term, 1) != 1 ||

2121 + if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) < 0 ||

2122 + write_in_full(fd, &term, 1) < 0 ||

2123 close_ref(lock) < 0) {

2124 strbuf_addf(err,

2125 "couldn't write '%s'", get_lock_file_path(lock->lk));

@@ -3338,8 +3338,8 @@ static int files_reflog_expire(struct ref_store *ref_store,

3338 strerror(errno));

3339 } else if (update &&

3340 (write_in_full(get_lock_file_fd(lock->lk),

3341 - oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||

3342 - write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 ||

3341 + oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) < 0 ||

3342 + write_str_in_full(get_lock_file_fd(lock->lk), "\n") < 0 ||

3343 close_ref(lock) < 0)) {

3344 status |= error("couldn't write %s",

3345 get_lock_file_path(lock->lk));

rerere.c

+1 -1

@@ -258,7 +258,7 @@ static int write_rr(struct string_list *rr, int out_fd)

258 rerere_id_hex(id),

259 rr->items[i].string, 0);

260

261 - if (write_in_full(out_fd, buf.buf, buf.len) != buf.len)

261 + if (write_in_full(out_fd, buf.buf, buf.len) < 0)

262 die("unable to write rerere record");

263

264 strbuf_release(&buf);

shallow.c

+3 -3

@@ -296,7 +296,7 @@ const char *setup_temporary_shallow(const struct oid_array *extra)

296 if (write_shallow_commits(&sb, 0, extra)) {

297 fd = xmks_tempfile(&temporary_shallow, git_path("shallow_XXXXXX"));

298

299 - if (write_in_full(fd, sb.buf, sb.len) != sb.len)

299 + if (write_in_full(fd, sb.buf, sb.len) < 0)

300 die_errno("failed to write to %s",

301 get_tempfile_path(&temporary_shallow));

302 close_tempfile(&temporary_shallow);

@@ -321,7 +321,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,

321 LOCK_DIE_ON_ERROR);

322 check_shallow_file_for_update();

323 if (write_shallow_commits(&sb, 0, extra)) {

324 - if (write_in_full(fd, sb.buf, sb.len) != sb.len)

324 + if (write_in_full(fd, sb.buf, sb.len) < 0)

325 die_errno("failed to write to %s",

326 get_lock_file_path(shallow_lock));

327 *alternate_shallow_file = get_lock_file_path(shallow_lock);

@@ -368,7 +368,7 @@ void prune_shallow(int show_only)

368 LOCK_DIE_ON_ERROR);

369 check_shallow_file_for_update();

370 if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {

371 - if (write_in_full(fd, sb.buf, sb.len) != sb.len)

371 + if (write_in_full(fd, sb.buf, sb.len) < 0)

372 die_errno("failed to write to %s",

373 get_lock_file_path(&shallow_lock));

374 commit_lock_file(&shallow_lock);

t/helper/test-delta.c

+1 -1

@@ -69,7 +69,7 @@ int cmd_main(int argc, const char **argv)

69 }

71 fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);

72 - if (fd < 0 || write_in_full(fd, out_buf, out_size) != out_size) {

72 + if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {

73 perror(argv[4]);

74 return 1;

75 }

transport-helper.c

+2 -3

@@ -44,8 +44,7 @@ static void sendline(struct helper_data *helper, struct strbuf *buffer)

44 {

45 if (debug)

46 fprintf(stderr, "Debug: Remote helper: -> %s", buffer->buf);

47 - if (write_in_full(helper->helper->in, buffer->buf, buffer->len)

48 - != buffer->len)

47 + if (write_in_full(helper->helper->in, buffer->buf, buffer->len) < 0)

48 die_errno("Full write to remote helper failed");

49 }

@@ -74,7 +73,7 @@ static void write_constant(int fd, const char *str)

73 {

74 if (debug)

75 fprintf(stderr, "Debug: Remote helper: -> %s", str);

77 - if (write_in_full(fd, str, strlen(str)) != strlen(str))

76 + if (write_in_full(fd, str, strlen(str)) < 0)

77 die_errno("Full write to remote helper failed");

78 }

wrapper.c

+1 -1

@@ -652,7 +652,7 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)

652 void write_file_buf(const char *path, const char *buf, size_t len)

653 {

654 int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);

655 - if (write_in_full(fd, buf, len) != len)

655 + if (write_in_full(fd, buf, len) < 0)

656 die_errno(_("could not write to %s"), path);

657 if (close(fd))

658 die_errno(_("could not close %s"), path);