git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: demerphq <demerphq@gmail.com>
Cc: Git <git@vger.kernel.org>, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v2] config: avoid "write_in_full(fd, buf, len) < len" pattern
Date: Wed, 13 Sep 2017 14:15:16 -0400	[thread overview]
Message-ID: <20170913181515.p7u2ouine3ysblqc@sigill.intra.peff.net> (raw)
In-Reply-To: <20170913171104.yu7ags4aq2zdwz6r@sigill.intra.peff.net>

On Wed, Sep 13, 2017 at 01:11:04PM -0400, Jeff King wrote:

> I scoured the code base for cases of this, but it turns out
> that these two in git_config_set_multivar_in_file_gently()
> are the only ones. This case is actually quite interesting:
> we don't have a size_t, but rather use the subtraction of
> two pointers. Which you might think would be a signed
> ptrdiff_t, but clearly both gcc and clang treat it as
> unsigned (possibly because the conditional just above
> guarantees that the result is greater than zero).

Sorry, this is totally wrong. As Jonathan pointed out (and as I was
puzzled over here), the result of a pointer subtraction _is_ a signed
type, and works fine.

What I missed is that copy_begin and copy_end here are actually size_t
variables, not the pointers. Sorry for the confusion, and here's an
updated version of the patch with this paragraph amended (the patch
itself is identical):

-- >8 --
Subject: [PATCH] config: avoid "write_in_full(fd, buf, len) < len" pattern

The return type of write_in_full() is a signed ssize_t,
because we may return "-1" on failure (even if we succeeded
in writing some bytes). But "len" itself is may be an
unsigned type (the function takes a size_t, but of course we
may have something else in the calling function). So while
it seems like:

  if (write_in_full(fd, buf, len) < len)
	die_errno("write error");

would trigger on error, it won't if "len" is unsigned.  The
compiler sees a signed/unsigned comparison and promotes the
signed value, resulting in (size_t)-1, the highest possible
size_t (or again, whatever type the caller has). This cannot
possibly be smaller than "len", and so the conditional can
never trigger.

I scoured the code base for cases of this, but it turns out
that these two in git_config_set_multivar_in_file_gently()
are the only ones. Here our "len" is the difference between
two size_t variables, making the result an unsigned size_t.
We can fix this by just checking for a negative return value
directly, as write_in_full() will never return any value
except -1 or the full count.

There's no addition to the test suite here, since you need
to convince write() to fail in order to see the problem. The
simplest reproduction recipe I came up with is to trigger
ENOSPC:

  # make a limited-size filesystem
  dd if=/dev/zero of=small.disk bs=1M count=1
  mke2fs small.disk
  mkdir mnt
  sudo mount -o loop small.disk mnt
  cd mnt
  sudo chown $USER:$USER .

  # make a config file with some content
  git config --file=config one.key value
  git config --file=config two.key value

  # now fill up the disk
  dd if=/dev/zero of=fill

  # and try to delete a key, which requires copying the rest
  # of the file to config.lock, and will fail on write()
  git config --file=config --unset two.key

That final command should (and does after this patch)
produce an error message due to the failed write, and leave
the file intact. Instead, it silently ignores the failure
and renames config.lock into place, leaving you with a
totally empty config file!

Reported-by: demerphq <demerphq@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 config.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/config.c b/config.c
index d0d8ce823a..eee4ac0355 100644
--- a/config.c
+++ b/config.c
@@ -2608,8 +2608,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 			/* write the first part of the config */
 			if (copy_end > copy_begin) {
 				if (write_in_full(fd, contents + copy_begin,
-						  copy_end - copy_begin) <
-				    copy_end - copy_begin)
+						  copy_end - copy_begin) < 0)
 					goto write_err_out;
 				if (new_line &&
 				    write_str_in_full(fd, "\n") != 1)
@@ -2631,8 +2630,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 		/* write the rest of the config */
 		if (copy_begin < contents_sz)
 			if (write_in_full(fd, contents + copy_begin,
-					  contents_sz - copy_begin) <
-			    contents_sz - copy_begin)
+					  contents_sz - copy_begin) < 0)
 				goto write_err_out;
 
 		munmap(contents, contents_sz);
-- 
2.14.1.874.ge7b2e05270


  parent reply	other threads:[~2017-09-13 18:15 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-13 11:59 Bug: git branch --unset-upstream command can nuke config when disk is full demerphq
2017-09-13 12:34 ` Jeff King
2017-09-13 13:38   ` demerphq
2017-09-13 14:17     ` Jeff King
2017-09-13 14:49       ` demerphq
2017-09-13 14:51         ` Jeff King
2017-09-13 15:18           ` demerphq
2017-09-13 15:22             ` Jeff King
2017-09-13 15:49               ` demerphq
2017-09-13 17:08 ` [PATCH 0/7] config.c may fail to notice some write() failures Jeff King
2017-09-13 17:11   ` [PATCH 1/7] config: avoid "write_in_full(fd, buf, len) < len" pattern Jeff King
2017-09-13 17:47     ` Jonathan Nieder
2017-09-13 17:53       ` Jeff King
2017-09-13 17:59         ` Jonathan Nieder
2017-09-13 18:11           ` Jeff King
2017-09-13 18:15     ` Jeff King [this message]
2017-09-13 18:24       ` [PATCH v2] " Jonathan Nieder
2017-09-13 18:58         ` Jeff King
2017-09-13 19:18           ` Jonathan Nieder
2017-09-13 19:49           ` Jonathan Nieder
2017-09-13 22:43           ` Ramsay Jones
2017-09-13 23:31             ` Ramsay Jones
2017-09-15  0:37               ` Jeff King
2017-09-15 15:15                 ` Ramsay Jones
2017-09-13 21:33         ` Junio C Hamano
2017-09-13 17:11   ` [PATCH 2/7] get-tar-commit-id: check write_in_full() return against 0 Jeff King
2017-09-13 17:53     ` Jonathan Nieder
2017-09-13 18:02       ` Jeff King
2017-09-13 18:37         ` Jeff King
2017-09-13 21:09     ` Jonathan Nieder
2017-09-15  0:40       ` Jeff King
2017-09-13 17:16   ` [PATCH 3/7] avoid "write_in_full(fd, buf, len) != len" pattern Jeff King
2017-09-13 21:14     ` Jonathan Nieder
2017-09-15  0:42       ` Jeff King
2017-09-13 17:16   ` [PATCH 4/7] convert less-trivial versions of "write_in_full() != len" Jeff King
2017-09-13 21:16     ` Jonathan Nieder
2017-09-13 17:17   ` [PATCH 5/7] pkt-line: check write_in_full() errors against "< 0" Jeff King
2017-09-13 21:17     ` Jonathan Nieder
2017-09-13 17:17   ` [PATCH 6/7] notes-merge: use ssize_t for write_in_full() return value Jeff King
2017-09-13 21:20     ` Jonathan Nieder
2017-09-15  0:43       ` Jeff King
2017-09-13 17:17   ` [PATCH 7/7] config: flip return value of store_write_*() Jeff King
2017-09-13 21:25     ` Jonathan Nieder
2017-09-15  0:46       ` Jeff King
2017-09-13 18:47   ` [PATCH 8/7] read_pack_header: handle signed/unsigned comparison in read result Jeff King
2017-09-13 19:11     ` Jonathan Nieder

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170913181515.p7u2ouine3ysblqc@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=demerphq@gmail.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).