[PATCH] Teach "git add" and friends to be paranoid

Subsystems: the rest

STALE3733d

14 messages, 9 authors, 2016-06-15 · open the first message on its own page

[PATCH] Teach "git add" and friends to be paranoid

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:18

When creating a loose object, we normally mmap(2) the entire file, and
hash and then compress to write it out in two separate steps for
efficiency.

This is perfectly good for the intended use of git---nobody is supposed to
be insane enough to expect that it won't break anything to muck with the
contents of a file after telling git to index it and before getting the
control back from git.

But the nature of breakage caused by such an abuse is rather bad.  We will
end up with loose object files, whose names do not match what are stored
and recovered when uncompressed.

This teaches the index_mem() codepath to be paranoid and hash and compress
the data after reading it in core.  The contents hashed may not match the
contents of the file in an insane use case, but at least this way the
result will be internally consistent.

Signed-off-by: Junio C Hamano <redacted>
---
 sha1_file.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 66 insertions(+), 15 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 657825e..d8a7722 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2278,7 +2278,8 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)
 }
 
 static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
-			      void *buf, unsigned long len, time_t mtime)
+			      void *buf, unsigned long len, time_t mtime,
+			      int paranoid)
 {
 	int fd, ret;
 	size_t size;
@@ -2286,6 +2287,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
 	z_stream stream;
 	char *filename;
 	static char tmpfile[PATH_MAX];
+	git_SHA_CTX ctx;
 
 	filename = sha1_file_name(sha1);
 	fd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);
@@ -2312,12 +2314,41 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
 	stream.next_in = (unsigned char *)hdr;
 	stream.avail_in = hdrlen;
 	while (deflate(&stream, 0) == Z_OK)
-		/* nothing */;
+		; /* nothing */
 
 	/* Then the data itself.. */
-	stream.next_in = buf;
-	stream.avail_in = len;
-	ret = deflate(&stream, Z_FINISH);
+	if (paranoid) {
+		unsigned char stablebuf[262144];
+		char *bufptr = buf;
+		unsigned long remainder = len;
+
+		git_SHA1_Init(&ctx);
+		git_SHA1_Update(&ctx, hdr, hdrlen);
+
+		ret = Z_OK;
+		while (remainder) {
+			unsigned long chunklen = remainder;
+
+			if (sizeof(stablebuf) <= chunklen)
+				chunklen = sizeof(stablebuf);
+			memcpy(stablebuf, bufptr, chunklen);
+			git_SHA1_Update(&ctx, stablebuf, chunklen);
+			stream.next_in = stablebuf;
+			stream.avail_in = chunklen;
+			do {
+				ret = deflate(&stream, Z_NO_FLUSH);
+			} while (ret == Z_OK);
+			bufptr += chunklen;
+			remainder -= chunklen;
+		}
+		if (ret != Z_STREAM_END)
+			ret = deflate(&stream, Z_FINISH);
+	} else {
+		stream.next_in = buf;
+		stream.avail_in = len;
+		ret = deflate(&stream, Z_FINISH);
+	}
+
 	if (ret != Z_STREAM_END)
 		die("unable to deflate new object %s (%d)", sha1_to_hex(sha1), ret);
 
@@ -2327,6 +2358,12 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
 
 	size = stream.total_out;
 
+	if (paranoid) {
+		unsigned char paranoid_sha1[20];
+		git_SHA1_Final(paranoid_sha1, &ctx);
+		if (hashcmp(paranoid_sha1, sha1))
+			die("hashed file is volatile");
+	}
 	if (write_buffer(fd, compressed, size) < 0)
 		die("unable to write sha1 file");
 	close_sha1_file(fd);
@@ -2344,7 +2381,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
 	return move_temp_to_file(tmpfile, filename);
 }
 
-int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)
+static int write_sha1_file_paranoid(void *buf, unsigned long len, const char *type, unsigned char *returnsha1, int paranoid)
 {
 	unsigned char sha1[20];
 	char hdr[32];
@@ -2358,7 +2395,12 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha
 		hashcpy(returnsha1, sha1);
 	if (has_sha1_file(sha1))
 		return 0;
-	return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);
+	return write_loose_object(sha1, hdr, hdrlen, buf, len, 0, paranoid);
+}
+
+int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)
+{
+	return write_sha1_file_paranoid(buf, len, type, returnsha1, 0);
 }
 
 int force_object_loose(const unsigned char *sha1, time_t mtime)
@@ -2376,7 +2418,7 @@ int force_object_loose(const unsigned char *sha1, time_t mtime)
 	if (!buf)
 		return error("cannot read sha1_file for %s", sha1_to_hex(sha1));
 	hdrlen = sprintf(hdr, "%s %lu", typename(type), len) + 1;
-	ret = write_loose_object(sha1, hdr, hdrlen, buf, len, mtime);
+	ret = write_loose_object(sha1, hdr, hdrlen, buf, len, mtime, 0);
 	free(buf);
 
 	return ret;
@@ -2405,10 +2447,15 @@ int has_sha1_file(const unsigned char *sha1)
 	return has_loose_object(sha1);
 }
 
+#define INDEX_MEM_WRITE_OBJECT  01
+#define INDEX_MEM_PARANOID      02
+
 static int index_mem(unsigned char *sha1, void *buf, size_t size,
-		     int write_object, enum object_type type, const char *path)
+		     enum object_type type, const char *path, int flag)
 {
 	int ret, re_allocated = 0;
+	int write_object = flag & INDEX_MEM_WRITE_OBJECT;
+	int paranoid = flag & INDEX_MEM_PARANOID;
 
 	if (!type)
 		type = OBJ_BLOB;
@@ -2426,9 +2473,11 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,
 	}
 
 	if (write_object)
-		ret = write_sha1_file(buf, size, typename(type), sha1);
+		ret = write_sha1_file_paranoid(buf, size, typename(type),
+					       sha1, paranoid);
 	else
 		ret = hash_sha1_file(buf, size, typename(type), sha1);
+
 	if (re_allocated)
 		free(buf);
 	return ret;
@@ -2437,23 +2486,25 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,
 int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 	     enum object_type type, const char *path)
 {
-	int ret;
+	int ret, flag;
 	size_t size = xsize_t(st->st_size);
 
+	flag = write_object ? INDEX_MEM_WRITE_OBJECT : 0;
 	if (!S_ISREG(st->st_mode)) {
 		struct strbuf sbuf = STRBUF_INIT;
 		if (strbuf_read(&sbuf, fd, 4096) >= 0)
-			ret = index_mem(sha1, sbuf.buf, sbuf.len, write_object,
-					type, path);
+			ret = index_mem(sha1, sbuf.buf, sbuf.len,
+					type, path, flag);
 		else
 			ret = -1;
 		strbuf_release(&sbuf);
 	} else if (size) {
 		void *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
-		ret = index_mem(sha1, buf, size, write_object, type, path);
+		flag |= INDEX_MEM_PARANOID;
+		ret = index_mem(sha1, buf, size, type, path, flag);
 		munmap(buf, size);
 	} else
-		ret = index_mem(sha1, NULL, size, write_object, type, path);
+		ret = index_mem(sha1, NULL, size, type, path, flag);
 	close(fd);
 	return ret;
 }
-- 
1.7.0.81.g58679

Re: [PATCH] Teach "git add" and friends to be paranoid

From: Jeff King <hidden>
Date: 2016-06-15 22:48:18

On Wed, Feb 17, 2010 at 05:16:23PM -0800, Junio C Hamano wrote:
+	if (paranoid) {
+		unsigned char stablebuf[262144];
Is 256K a bit big for allocating on the stack? Modern OS's seem to give
us at least a couple of megabytes (my Linux boxen all have 8M, and
even Solaris 8 seems to have that much). But PTHREAD_STACK_MIN is only
16K (I don't think it is possible to hit this code path in a thread
right now, but I'm not sure). And I have no idea what the situation is
on Windows.

I dunno if it is worth worrying about, but maybe somebody more clueful
than me can comment.

-Peff

Re: [PATCH] Teach "git add" and friends to be paranoid

From: Nicolas Pitre <nico@fluxnic.net>
Date: 2016-06-15 22:48:18

On Wed, 17 Feb 2010, Jeff King wrote:
On Wed, Feb 17, 2010 at 05:16:23PM -0800, Junio C Hamano wrote:
quoted
+	if (paranoid) {
+		unsigned char stablebuf[262144];
Is 256K a bit big for allocating on the stack? Modern OS's seem to give
us at least a couple of megabytes (my Linux boxen all have 8M, and
even Solaris 8 seems to have that much). But PTHREAD_STACK_MIN is only
16K (I don't think it is possible to hit this code path in a thread
right now, but I'm not sure). And I have no idea what the situation is
on Windows.

I dunno if it is worth worrying about, but maybe somebody more clueful
than me can comment.
It is likely to have better performance if the buffer is small enough to 
fit in the CPU L1 cache.  There are two sequencial passes over the 
buffer: one for the SHA1 computation, and another for the compression, 
and currently they're sure to trash the L1 cache on each pass.

Of course that requires big enough objects to matter.


Nicolas

Re: [PATCH] Teach "git add" and friends to be paranoid

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:18

Nicolas Pitre [off-list ref] writes:
It is likely to have better performance if the buffer is small enough to 
fit in the CPU L1 cache.  There are two sequencial passes over the 
buffer: one for the SHA1 computation, and another for the compression, 
and currently they're sure to trash the L1 cache on each pass.
I did a very unscientific test to hash about 14k paths (arch/ and fs/ from
the kernel source) using "git-hash-object -w --stdin-paths" into an empty
repository with varying sizes of paranoia buffer (quarter, 1, 4, 8 and
256kB) and saw 8-30% overhead.  256kB did hurt and around 4kB seemed to be
optimal for my this small sample load.

In any case, with any size of paranoia, this hurts the sane use case, so
I'd introduce an expert switch to disable it, like this.

-- >8 --
When creating a loose object, we normally mmap(2) the entire file, and
hash and then compress to write it out in two separate steps for
efficiency.

This is perfectly good for the intended use of git---nobody is supposed to
be insane enough to expect that it won't break anything to muck with the
contents of a file after telling git to index it and before getting the
control back from git.

But the nature of breakage caused by such an abuse is rather bad.  We will
end up with loose object files, whose names do not match what are stored
and recovered when uncompressed.

This teaches the index_mem() codepath to be paranoid and hash and compress
the data after reading it in core.  The contents hashed may not match the
contents of the file in an insane use case, but at least this way the
result will be internally consistent.

People with saner use of git can regain performance by setting a new
configuration variable 'core.volatilefiles' to false to disable this
check.

Signed-off-by: Junio C Hamano <redacted>
---
 Documentation/config.txt |    7 ++++
 cache.h                  |    1 +
 config.c                 |    6 +++
 environment.c            |    1 +
 sha1_file.c              |   83 +++++++++++++++++++++++++++++++++++++--------
 5 files changed, 83 insertions(+), 15 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 52786c7..0295aee 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -117,6 +117,14 @@ core.fileMode::
 	the working copy are ignored; useful on broken filesystems like FAT.
 	See linkgit:git-update-index[1]. True by default.
 
+core.volatilefiles::
+	If you modify a file after telling git to record it (e.g. with
+	"git add") but before git finishes the request and gives the
+	control back to you, you may create a broken object (and of course
+	you can keep both halves ;-).  Setting this option to true will
+	tell git to be extra careful to detect the situation and abort.
+	Defaults to true.
+
 core.ignoreCygwinFSTricks::
 	This option is only used by Cygwin implementation of Git. If false,
 	the Cygwin stat() and lstat() functions are used. This may be useful
diff --git a/cache.h b/cache.h
index 231c06d..e5a87cf 100644
--- a/cache.h
+++ b/cache.h
@@ -497,6 +497,7 @@ extern int trust_ctime;
 extern int quote_path_fully;
 extern int has_symlinks;
 extern int ignore_case;
+extern int worktree_files_are_volatile;
 extern int assume_unchanged;
 extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
diff --git a/config.c b/config.c
index 790405a..9898041 100644
--- a/config.c
+++ b/config.c
@@ -360,6 +360,12 @@ static int git_default_core_config(const char *var, const char *value)
 		trust_executable_bit = git_config_bool(var, value);
 		return 0;
 	}
+
+	if (!strcmp(var, "core.volatilefiles")) {
+		worktree_files_are_volatile = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "core.trustctime")) {
 		trust_ctime = git_config_bool(var, value);
 		return 0;
diff --git a/environment.c b/environment.c
index e278bce..5d0faf3 100644
--- a/environment.c
+++ b/environment.c
@@ -22,6 +22,7 @@ int is_bare_repository_cfg = -1; /* unspecified */
 int log_all_ref_updates = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
 int repository_format_version;
+int worktree_files_are_volatile = 1; /* yuck */
 const char *git_commit_encoding;
 const char *git_log_output_encoding;
 int shared_repository = PERM_UMASK;
diff --git a/sha1_file.c b/sha1_file.c
index 52d1ead..e126179 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2335,14 +2335,18 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)
 }
 
 static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
-			      void *buf, unsigned long len, time_t mtime)
+			      void *buf, unsigned long len, time_t mtime,
+			      int paranoid)
 {
 	int fd, size, ret;
 	unsigned char *compressed;
 	z_stream stream;
 	char *filename;
 	static char tmpfile[PATH_MAX];
+	git_SHA_CTX ctx;
 
+	if (!worktree_files_are_volatile)
+		paranoid = 0;
 	filename = sha1_file_name(sha1);
 	fd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);
 	if (fd < 0) {
@@ -2366,12 +2370,41 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
 	stream.next_in = (unsigned char *)hdr;
 	stream.avail_in = hdrlen;
 	while (deflate(&stream, 0) == Z_OK)
-		/* nothing */;
+		; /* nothing */
 
 	/* Then the data itself.. */
-	stream.next_in = buf;
-	stream.avail_in = len;
-	ret = deflate(&stream, Z_FINISH);
+	if (paranoid) {
+		unsigned char stablebuf[4096];
+		char *bufptr = buf;
+		unsigned long remainder = len;
+
+		git_SHA1_Init(&ctx);
+		git_SHA1_Update(&ctx, hdr, hdrlen);
+
+		ret = Z_OK;
+		while (remainder) {
+			unsigned long chunklen = remainder;
+
+			if (sizeof(stablebuf) <= chunklen)
+				chunklen = sizeof(stablebuf);
+			memcpy(stablebuf, bufptr, chunklen);
+			git_SHA1_Update(&ctx, stablebuf, chunklen);
+			stream.next_in = stablebuf;
+			stream.avail_in = chunklen;
+			do {
+				ret = deflate(&stream, Z_NO_FLUSH);
+			} while (ret == Z_OK);
+			bufptr += chunklen;
+			remainder -= chunklen;
+		}
+		if (ret != Z_STREAM_END)
+			ret = deflate(&stream, Z_FINISH);
+	} else {
+		stream.next_in = buf;
+		stream.avail_in = len;
+		ret = deflate(&stream, Z_FINISH);
+	}
+
 	if (ret != Z_STREAM_END)
 		die("unable to deflate new object %s (%d)", sha1_to_hex(sha1), ret);
 
@@ -2381,6 +2414,12 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
 
 	size = stream.total_out;
 
+	if (paranoid) {
+		unsigned char paranoid_sha1[20];
+		git_SHA1_Final(paranoid_sha1, &ctx);
+		if (hashcmp(paranoid_sha1, sha1))
+			die("hashed file is volatile");
+	}
 	if (write_buffer(fd, compressed, size) < 0)
 		die("unable to write sha1 file");
 	close_sha1_file(fd);
@@ -2398,7 +2437,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
 	return move_temp_to_file(tmpfile, filename);
 }
 
-int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)
+static int write_sha1_file_paranoid(void *buf, unsigned long len, const char *type, unsigned char *returnsha1, int paranoid)
 {
 	unsigned char sha1[20];
 	char hdr[32];
@@ -2412,7 +2451,12 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha
 		hashcpy(returnsha1, sha1);
 	if (has_sha1_file(sha1))
 		return 0;
-	return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);
+	return write_loose_object(sha1, hdr, hdrlen, buf, len, 0, paranoid);
+}
+
+int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)
+{
+	return write_sha1_file_paranoid(buf, len, type, returnsha1, 0);
 }
 
 int force_object_loose(const unsigned char *sha1, time_t mtime)
@@ -2430,7 +2474,7 @@ int force_object_loose(const unsigned char *sha1, time_t mtime)
 	if (!buf)
 		return error("cannot read sha1_file for %s", sha1_to_hex(sha1));
 	hdrlen = sprintf(hdr, "%s %lu", typename(type), len) + 1;
-	ret = write_loose_object(sha1, hdr, hdrlen, buf, len, mtime);
+	ret = write_loose_object(sha1, hdr, hdrlen, buf, len, mtime, 0);
 	free(buf);
 
 	return ret;
@@ -2467,10 +2511,15 @@ int has_sha1_file(const unsigned char *sha1)
 	return has_loose_object(sha1);
 }
 
+#define INDEX_MEM_WRITE_OBJECT  01
+#define INDEX_MEM_PARANOID      02
+
 static int index_mem(unsigned char *sha1, void *buf, size_t size,
-		     int write_object, enum object_type type, const char *path)
+		     enum object_type type, const char *path, int flag)
 {
 	int ret, re_allocated = 0;
+	int write_object = flag & INDEX_MEM_WRITE_OBJECT;
+	int paranoid = flag & INDEX_MEM_PARANOID;
 
 	if (!type)
 		type = OBJ_BLOB;
@@ -2488,9 +2537,11 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,
 	}
 
 	if (write_object)
-		ret = write_sha1_file(buf, size, typename(type), sha1);
+		ret = write_sha1_file_paranoid(buf, size, typename(type),
+					       sha1, paranoid);
 	else
 		ret = hash_sha1_file(buf, size, typename(type), sha1);
+
 	if (re_allocated)
 		free(buf);
 	return ret;
@@ -2499,23 +2550,25 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,
 int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 	     enum object_type type, const char *path)
 {
-	int ret;
+	int ret, flag;
 	size_t size = xsize_t(st->st_size);
 
+	flag = write_object ? INDEX_MEM_WRITE_OBJECT : 0;
 	if (!S_ISREG(st->st_mode)) {
 		struct strbuf sbuf = STRBUF_INIT;
 		if (strbuf_read(&sbuf, fd, 4096) >= 0)
-			ret = index_mem(sha1, sbuf.buf, sbuf.len, write_object,
-					type, path);
+			ret = index_mem(sha1, sbuf.buf, sbuf.len,
+					type, path, flag);
 		else
 			ret = -1;
 		strbuf_release(&sbuf);
 	} else if (size) {
 		void *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
-		ret = index_mem(sha1, buf, size, write_object, type, path);
+		flag |= INDEX_MEM_PARANOID;
+		ret = index_mem(sha1, buf, size, type, path, flag);
 		munmap(buf, size);
 	} else
-		ret = index_mem(sha1, NULL, size, write_object, type, path);
+		ret = index_mem(sha1, NULL, size, type, path, flag);
 	close(fd);
 	return ret;
 }
-- 
1.7.0.81.g58679

Re: [PATCH] Teach "git add" and friends to be paranoid

From: Wincent Colaiuta <hidden>
Date: 2016-06-15 22:48:18

El 18/02/2010, a las 06:36, Junio C Hamano escribió:
Nicolas Pitre [off-list ref] writes:
quoted
It is likely to have better performance if the buffer is small enough to 
fit in the CPU L1 cache.  There are two sequencial passes over the 
buffer: one for the SHA1 computation, and another for the compression, 
and currently they're sure to trash the L1 cache on each pass.
I did a very unscientific test to hash about 14k paths (arch/ and fs/ from
the kernel source) using "git-hash-object -w --stdin-paths" into an empty
repository with varying sizes of paranoia buffer (quarter, 1, 4, 8 and
256kB) and saw 8-30% overhead.  256kB did hurt and around 4kB seemed to be
optimal for my this small sample load.

In any case, with any size of paranoia, this hurts the sane use case, so
I'd introduce an expert switch to disable it, like this.
Shouldn't a switch that hurts performance and is only needed for insane use cases default to off rather than on?

Cheers,
Wincent

Re: [PATCH] Teach "git add" and friends to be paranoid

From: Thomas Rast <hidden>
Date: 2016-06-15 22:48:18

On Thursday 18 February 2010 02:16:23 Junio C Hamano wrote:
When creating a loose object, we normally mmap(2) the entire file, and
hash and then compress to write it out in two separate steps for
efficiency.

This is perfectly good for the intended use of git---nobody is supposed to
be insane enough to expect that it won't break anything to muck with the
contents of a file after telling git to index it and before getting the
control back from git.
This makes it sound as if the user is to blame, but IMHO we're just
not checking the input well enough.  The user should never be able to
corrupt the repository (without git noticing!) just by running a git
command and manipulating the worktree in parallel.  The file data at
any given time is just user input, and you also cannot (I hope;
otherwise let's fix it!) corrupt the repository merely by typoing some
command arguments.

(Mucking around in .git is an entirely different matter, but that is
off limits.)
This teaches the index_mem() codepath to be paranoid and hash and compress
the data after reading it in core.  The contents hashed may not match the
contents of the file in an insane use case, but at least this way the
result will be internally consistent.
Doesn't that trigger on windows, where xmmap() already makes a copy?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] Teach "git add" and friends to be paranoid

From: Zygo Blaxell <hidden>
Date: 2016-06-15 22:48:18

On Thu, Feb 18, 2010 at 08:27:28AM +0100, Wincent Colaiuta wrote:
Shouldn't a switch that hurts performance and is only needed for insane use cases default to off rather than on?
While I don't disagree that default off might(*) be a good idea,
I do object to the categorization of this use case as 'insane'.

Neither the documentation for 'git add' nor its various aliases (e.g. 'git
commit' with paths or -a, etc) mentions that any use of 'git add'
might cause repository corruption under any circumstances.  Contrast with
examples of repository-corrupting pitfalls in the man pages of tools
such as 'git clone' and 'git gc'.

In fact, the language in the git add man page seems to suggest the
opposite, using words like "snapshot" and pointing out several times
that the index is intentionally immune to changes interleaved between
'git add' and 'git commit' commands.

Common sense (for Unix users) is that the index is not immune to changes
*during* git add, but nowhere in my wildest nightmares would I conceive
that changes in file contents during git add would *corrupt the
repository* and git would *fail to notice or give useful diagnostics*
until *days or weeks later* after the corruption has already *spread to
multiple repositories* through *git push with default options*.

Now, if you want to put that text in the man pages of 'git add' and
friends, and point out the paranoia switch there, I have nothing to
object to.

I also see nothing prohibiting concurrent file modification in some
reasonable revision-control use cases.  What happens if I do a 'git
commit -a' on, say, proprietary EDA tool data files or Microsoft Office
documents, and those tools choose an unfortunate moment to automatically
update files under revision control?  Granted, I can't really expect the
repo to contain usable data, but what I do expect is another commit, or
a rebased/amended commit, that fixes the mangled file's contents--not to
be required to rebase on the commit's parent everything that comes
after it, then purge my reflogs so 'git gc' will work again.

Working directories on network filesystems might do all kinds of strange
things, most of which aren't intentional.  It's one thing to commit a
useless tree, and quite another to unintentionally commit an irretrievable
one.

(*) "might" be a good idea because there's been some evidence to suggest
that a paranoid implementation of git add might perform better than the
mmap-based one in all cases, if more work was done than anyone seems
willing to do.

Re: [PATCH] Teach "git add" and friends to be paranoid

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:48:18

Zygo Blaxell wrote:
On Thu, Feb 18, 2010 at 08:27:28AM +0100, Wincent Colaiuta wrote:
quoted
Shouldn't a switch that hurts performance and is only needed for
insane use cases default to off rather than on?
While I don't disagree that default off might(*) be a good idea,
I do object to the categorization of this use case as 'insane'.
FWIW I think default off would not be a good idea.  This talk of
insane uses started from the idea that git is not so great for taking
automatic snapshots, but as you pointed out, other situations can
trigger this and the failure mode is pretty bad.
(*) "might" be a good idea because there's been some evidence to suggest
that a paranoid implementation of git add might perform better than the
mmap-based one in all cases, if more work was done than anyone seems
willing to do.
What you are saying here seems a bit handwavy.  If you have some
concrete ideas about what this paranoid implementation should look
like, I encourage you to write a rough patch.  The two patches so far
have indicated the relevant parts of sha1_file.c (index_fd at the
beginning and write_sha1_file at the end of the pipeline,
respectively).  Special cases include:

 - The blob being added to the repository is a special file (e.g.,
   pipe) specified on the 'git hash-object' command line: I think it’s
   fine if this is slow, but it should keep working.

 - The blob was generated in memory (e.g. 'git apply --cached').

 - autocrlf conversion is on.  This means scanning through the file to
   collect statistics on the dominant line ending, then scanning
   through again to convert the file.

 - some other filter is on.  This means sending the file as input to
   a command, then slurping it up somewhere until its length has been
   determined for the beginning of the blob header

 - The blob being added to the repository is already in the repository,
   so it would be a waste of time to compress and write it again.

Some of these already don’t have great performance for large files
(autocrlf and filters), and I suspect there is room for improvement
for many of them.

Jonathan

Re: [PATCH] Teach "git add" and friends to be paranoid

From: Dmitry Potapov <hidden>
Date: 2016-06-15 22:48:18

Hi Junio,

I am sorry I have not had time to reply earlier. I think it is possible
to avoid the overhead of being on the safe side in a few common cases.
Here is a patch. I have not had time to test it, but changes appear to
trivial.

-- >8 --
From 3e53610a41c4aad458dff13135a73bb4944f456b Mon Sep 17 00:00:00 2001
From: Dmitry Potapov <redacted>
Date: Fri, 19 Feb 2010 11:00:51 +0300
Subject: [PATCH] speed up "git add" by avoiding the paranoid mode

While the paranoid mode preserve the git repository from corruption in the
case when the added file is changed simultaneously with running "git add",
it has some overhead. However, in a few common cases, it is possible to
avoid this mode and still be on the safe side:

1. If mmap() is implemented as reading the whole file in memory.

2. If the whole file was read in memory as result of applying some filter.

3. If the added file is small, it is faster to use read() than mmap().

Signed-off-by: Dmitry Potapov <redacted>
---
 sha1_file.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index d8a7722..4efeb21 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2469,6 +2469,7 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,
 		                   write_object ? safe_crlf : 0)) {
 			buf = strbuf_detach(&nbuf, &size);
 			re_allocated = 1;
+			paranoid = 0;
 		}
 	}
 
@@ -2490,7 +2491,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 	size_t size = xsize_t(st->st_size);
 
 	flag = write_object ? INDEX_MEM_WRITE_OBJECT : 0;
-	if (!S_ISREG(st->st_mode)) {
+	if (!S_ISREG(st->st_mode) || size < 262144) {
 		struct strbuf sbuf = STRBUF_INIT;
 		if (strbuf_read(&sbuf, fd, 4096) >= 0)
 			ret = index_mem(sha1, sbuf.buf, sbuf.len,
@@ -2500,7 +2501,9 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 		strbuf_release(&sbuf);
 	} else if (size) {
 		void *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
+#ifndef NO_MMAP
 		flag |= INDEX_MEM_PARANOID;
+#endif
 		ret = index_mem(sha1, buf, size, type, path, flag);
 		munmap(buf, size);
 	} else
-- 
1.7.0

-- >8 --

Re: [PATCH] Teach "git add" and friends to be paranoid

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:18

Dmitry Potapov [off-list ref] writes:
... I think it is possible
to avoid the overhead of being on the safe side in a few common cases.
Here is a patch. I have not had time to test it, but changes appear to
trivial.
Yeah, these are obvious "paranoia not needed" cases.

How much "speed-up" are we talking about, though?  Can we quantify?  I
personally think it is not even worth to quantify it but instead simply
say "avoid unnecessary computation" without saying "speed up", though ;-)

Thanks.
quoted hunk
-- >8 --
From 3e53610a41c4aad458dff13135a73bb4944f456b Mon Sep 17 00:00:00 2001
From: Dmitry Potapov <redacted>
Date: Fri, 19 Feb 2010 11:00:51 +0300
Subject: [PATCH] speed up "git add" by avoiding the paranoid mode

While the paranoid mode preserve the git repository from corruption in the
case when the added file is changed simultaneously with running "git add",
it has some overhead. However, in a few common cases, it is possible to
avoid this mode and still be on the safe side:

1. If mmap() is implemented as reading the whole file in memory.

2. If the whole file was read in memory as result of applying some filter.

3. If the added file is small, it is faster to use read() than mmap().

Signed-off-by: Dmitry Potapov <redacted>
---
 sha1_file.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index d8a7722..4efeb21 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2469,6 +2469,7 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,
 		                   write_object ? safe_crlf : 0)) {
 			buf = strbuf_detach(&nbuf, &size);
 			re_allocated = 1;
+			paranoid = 0;
 		}
 	}
 
@@ -2490,7 +2491,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 	size_t size = xsize_t(st->st_size);
 
 	flag = write_object ? INDEX_MEM_WRITE_OBJECT : 0;
-	if (!S_ISREG(st->st_mode)) {
+	if (!S_ISREG(st->st_mode) || size < 262144) {
 		struct strbuf sbuf = STRBUF_INIT;
 		if (strbuf_read(&sbuf, fd, 4096) >= 0)
 			ret = index_mem(sha1, sbuf.buf, sbuf.len,
@@ -2500,7 +2501,9 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 		strbuf_release(&sbuf);
 	} else if (size) {
 		void *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
+#ifndef NO_MMAP
 		flag |= INDEX_MEM_PARANOID;
+#endif
 		ret = index_mem(sha1, buf, size, type, path, flag);
 		munmap(buf, size);
 	} else
-- 
1.7.0

-- >8 --

Re: [PATCH] Teach "git add" and friends to be paranoid

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:19

Junio C Hamano [off-list ref] writes:
Dmitry Potapov [off-list ref] writes:
quoted
... I think it is possible
to avoid the overhead of being on the safe side in a few common cases.
Here is a patch. I have not had time to test it, but changes appear to
trivial.
Yeah, these are obvious "paranoia not needed" cases.
Actually the "if it is smaller than 256k" part is not quite obvious.
quoted
@@ -2490,7 +2491,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 	size_t size = xsize_t(st->st_size);
 
 	flag = write_object ? INDEX_MEM_WRITE_OBJECT : 0;
-	if (!S_ISREG(st->st_mode)) {
+	if (!S_ISREG(st->st_mode) || size < 262144) {
 		struct strbuf sbuf = STRBUF_INIT;
 		if (strbuf_read(&sbuf, fd, 4096) >= 0)
 			ret = index_mem(sha1, sbuf.buf, sbuf.len,
INDEX_MEM_PARANOID is never given to index_mem() in this codepath, so
trade-offs look like this:

 * In non-paranoia mode, your conjecture is that between

   - malloc, read, SHA-1, deflate, and then free; and
   - mmap, SHA-1, deflate and then munmap

   the former is faster for small files that can fit in core without
   thrashing.

 * In paranoia mode, your conjecture is that between

   - malloc, read, SHA-1, deflate, and then free; and
   - mmap, SHA-1, SHA-1 and deflate in chunks, and then munmap

   the former is faster for small files that can fit in core without
   thrashing.

The "mmap" strategy has larger cost in paranoia mode compared to its cost
in non-paranoia mode.  The "read" strategy on the other hand has the same
cost in both modes.  If this "read small files" is good for non-paranoia
mode, it is obvious that it is also good (better) for paranoia mode.

Which means that this hunk addresses an unrelated issue.  "paranoid
avoidance" falls naturally as a side effect of doing this, but that is not
the primary effect of this change.

There needs some benchmarking to justify it, I think.

So I'd split this hunk out when queuing.

Thanks.

Re: [PATCH] Teach "git add" and friends to be paranoid

From: Dmitry Potapov <hidden>
Date: 2016-06-15 22:48:19

On Sat, Feb 20, 2010 at 11:23:11AM -0800, Junio C Hamano wrote:
There needs some benchmarking to justify it, I think.

So I'd split this hunk out when queuing.
I completely argee with your reasoning that it is a separate issue and
it needs some benchmarking to prove its usefulness. So, I have done it
today.

For that, I have created a repository up to 512Mb size and containing up
to 100,000 files (for small files the file number was the limiting
factor, for big files, the total size limit was used). I intentionally
limit the total size to 512Mb to make sure that they are in the FS cache
and no disk related effects.  The content of files have been generated
randomly by /dev/urandom and then used in all tests for one file size.
I have made 5 runs using mmap() (with git 1.7.0) and 5 runs with using
read() (after applying the patch below).

Below is the best result of 5 runs for each size. "Before" marks the
original version, which uses mmap(). "After" marks the modified version
using read(). The command used to measure time was:

cat list | time git hash-object --stdin-paths >/dev/null

So it is just calculating SHA-1 without deflating and writing the
result on the disk, which significantly depends on fsync() speed.

Tested on: Intel(R) Core(TM)2 Quad  CPU   Q9300  @ 2.50GHz

Here are results:

file size = 1Kb; Hashing 100000 files
Before:
0.63user 0.86system 0:01.49elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+100428minor)pagefaults 0swaps
After:
0.54user 0.53system 0:01.07elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+421minor)pagefaults 0swaps

file size = 2Kb; Hashing 100000 files
Before:
1.04user 0.79system 0:01.82elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+100428minor)pagefaults 0swaps
After:
0.95user 0.48system 0:01.43elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+422minor)pagefaults 0swaps

file size = 4Kb; Hashing 100000 files
Before:
1.73user 0.74system 0:02.47elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+100428minor)pagefaults 0swaps
After:
1.54user 0.57system 0:02.11elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+424minor)pagefaults 0swaps

file size = 8Kb; Hashing 64000 files
Before:
1.86user 0.63system 0:02.48elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+128428minor)pagefaults 0swaps
After:
1.75user 0.50system 0:02.23elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+424minor)pagefaults 0swaps

file size = 16Kb; Hashing 32000 files
Before:
1.73user 0.41system 0:02.14elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+128428minor)pagefaults 0swaps
After:
1.68user 0.32system 0:02.00elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+425minor)pagefaults 0swaps

file size = 32Kb; Hashing 16000 files
Before:
1.65user 0.32system 0:01.96elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+128428minor)pagefaults 0swaps
After:
1.64user 0.24system 0:01.87elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+431minor)pagefaults 0swaps

file size = 64Kb; Hashing 8000 files
Before:
1.71user 0.17system 0:01.87elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+128428minor)pagefaults 0swaps
After:
1.63user 0.18system 0:01.81elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+438minor)pagefaults 0swaps

file size = 128Kb; Hashing 4000 files
Before:
1.60user 0.20system 0:01.79elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+128428minor)pagefaults 0swaps
After:
1.55user 0.20system 0:01.75elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+454minor)pagefaults 0swaps

file size = 256Kb; Hashing 2000 files
Before:
1.62user 0.15system 0:01.77elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+128429minor)pagefaults 0swaps
After:
1.56user 0.16system 0:01.71elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+551minor)pagefaults 0swaps

file size = 512Kb; Hashing 1000 files
Before:
1.59user 0.17system 0:01.76elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+128428minor)pagefaults 0swaps
After:
1.56user 0.15system 0:01.71elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+679minor)pagefaults 0swaps

file size = 1024Kb; Hashing 500 files
Before:
1.64user 0.15system 0:01.78elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+128429minor)pagefaults 0swaps
After:
1.61user 0.15system 0:01.76elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+934minor)pagefaults 0swaps


As you can see, in all tests the read() version performed better than
mmap() though the difference decreases with increase of the file size.
While for 1Kb files, the speed up is 39% (based on the elapsed time),
it is mere 1% for 1Mb file size.

Note: I do not use strbuf_read(), because it is suboptimal to deal with
this case, because we know the size ahead. (In fact, strbuf_read() is
not so good even for unknown size as it does redundant strbuf_grow()
almost in every use case, which probably needs to be fixed).

-- >8 --
From 6b3f8335dece7c9b9f810b1ab08f1bcb090e4d5e Mon Sep 17 00:00:00 2001
From: Dmitry Potapov <redacted>
Date: Sun, 21 Feb 2010 09:32:19 +0300
Subject: [PATCH] hash-object: don't use mmap() for small files

Using read() instead of mmap() can be 39% speed up for 1Kb files and is
1% speed up 1Mb files. For larger files, it is better to use mmap(),
because the difference between is not significant, and when there is not
enough memory, mmap() performs much better, because it avoids swapping.

Signed-off-by: Dmitry Potapov <redacted>
---
 sha1_file.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 657825e..8a83e56 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2434,6 +2434,8 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,
 	return ret;
 }
 
+#define SMALL_FILE_SIZE (1024*1024)
+
 int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 	     enum object_type type, const char *path)
 {
@@ -2448,6 +2450,14 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 		else
 			ret = -1;
 		strbuf_release(&sbuf);
+	} else if (size <= SMALL_FILE_SIZE) {
+		char *buf = xmalloc(size);
+		if (size == read_in_full(fd, buf, size))
+			ret = index_mem(sha1, buf, size, write_object, type,
+					path);
+		else
+			ret = error("short read %s", strerror(errno));
+		free(buf);
 	} else if (size) {
 		void *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
 		ret = index_mem(sha1, buf, size, write_object, type, path);
-- 
1.7.0

-- >8 --


Thanks,
Dmitry

Re: [PATCH] Teach "git add" and friends to be paranoid

From: Paolo Bonzini <hidden>
Date: 2016-06-15 22:48:19

On 02/18/2010 06:36 AM, Junio C Hamano wrote:
Nicolas Pitre[off-list ref]  writes:
quoted
It is likely to have better performance if the buffer is small enough to
fit in the CPU L1 cache.  There are two sequencial passes over the
buffer: one for the SHA1 computation, and another for the compression,
and currently they're sure to trash the L1 cache on each pass.
I did a very unscientific test to hash about 14k paths (arch/ and fs/ from
the kernel source) using "git-hash-object -w --stdin-paths" into an empty
repository with varying sizes of paranoia buffer (quarter, 1, 4, 8 and
256kB) and saw 8-30% overhead.  256kB did hurt and around 4kB seemed to be
optimal for my this small sample load.

In any case, with any size of paranoia, this hurts the sane use case
Because by mmaping + memcpying you are getting the worst of both cases: 
you get a page fault per page like with mmap, and touch memory twice 
like with read.

Paolo

Re: [PATCH] Teach "git add" and friends to be paranoid

From: Dmitry Potapov <hidden>
Date: 2016-06-15 22:48:19

On Mon, Feb 22, 2010 at 01:59:50PM +0100, Paolo Bonzini wrote:
On 02/18/2010 06:36 AM, Junio C Hamano wrote:
quoted
In any case, with any size of paranoia, this hurts the sane use case
Because by mmaping + memcpying you are getting the worst of both
cases: you get a page fault per page like with mmap, and touch
memory twice like with read.
and there is also an extra round of SHA-1 calculation, which I believe
is more expensive than memcpy().


Dmitry
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help