[PATCH] refs.c: add a function to append a reflog entry to a fd

[Stefan Beller <sbeller@google.com>]

Break out the code to create the string and writing it to the file
descriptor from log_ref_write and add it into a dedicated function
log_ref_write_fd. For now this is only used from log_ref_write,
but later on we will call this function from reflog transactions too,
which means that we will end up with only a single place,
where we write a reflog entry to a file instead of the current two
places (log_ref_write and builtin/reflog.c).

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 48 ++++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 18 deletions(-)
 
Sorry for the small change w.r.t. sprintf/snprintf
Here comes a resend without changing code, but just making it a new function,
so we come forwards with the patch.


Let's discuss the change and decide if I shouldsend a follow up patch to change 
it into snprintf.

On Wed, Nov 19, 2014 at 5:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> +     logrec = xmalloc(maxlen);
>> +     len = snprintf(logrec, maxlen, "%s %s %s\n",
>> +                    sha1_to_hex(old_sha1),
>> +                    sha1_to_hex(new_sha1),
>> +                    committer);
>> +     if (msglen)
>> +             len += copy_msg(logrec + len - 1, msg) - 1;
>
> In this codepath, you are allocating enough buffer to hold the whole
> message; there is no difference between sprintf() and snprintf().
> If the difference mattered, you would have chopped the reflog entry
> too short, and produced a wrong result, but you then discard the
> whole record (the code that follows the above), losing data.

Hypothetically speaking:

There should be no difference between sprintf() and snprintf(). As soon as you
would have a difference, you'd risk stomping on other peoples data, which could lead
to code insertion, iff the format of the inserted data is long enough overwriting
parts of your stack? Or you could change data to alter the program flow to direct
the program to another easier abusable bug.
Does this make sense? This is about protecting a user from a malicious attacker.

> imagine you intended to copy "rm -fr ./tmpdir" and by mistake
> you stopped at "rm -fr ./" @~@).

Yes, that's when you should check the return code of snprintf and compare to 
strlen("rm -fr ./tmpdir"). That kind of bug also has severe consequences, but
it's not yet involving a malicious attacker, so I'd file this as a safety instead  
of a security flaw.

Safety related bugs are easier to find usually as you don't need to have to
think about what weird things an attacker might do. This kind of bug can be found
directly from reading the code and its surroundings 
("there it should print 'rm -fr ./tmpdir'... Gah! I did not check the return value")


diff --git a/refs.c b/refs.c
index 5ff457e..f9b42e5 100644
--- a/refs.c
+++ b/refs.c
@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)
 	return 0;
 }
 
+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
+			    const unsigned char *new_sha1,
+			    const char *committer, const char *msg)
+{
+	int msglen, written;
+	unsigned maxlen, len;
+	char *logrec;
+
+	msglen = msg ? strlen(msg) : 0;
+	maxlen = strlen(committer) + msglen + 100;
+	logrec = xmalloc(maxlen);
+	len = sprintf(logrec, maxlen, "%s %s %s\n",
+		      sha1_to_hex(old_sha1),
+		      sha1_to_hex(new_sha1),
+		      committer);
+	if (msglen)
+		len += copy_msg(logrec + len - 1, msg) - 1;
+
+	written = len <= maxlen ? write_in_full(fd, logrec, len) : -1;
+	free(logrec);
+	if (written != len)
+		return -1;
+
+	return 0;
+}
+
 static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 			 const unsigned char *new_sha1, const char *msg)
 {
-	int logfd, result, written, oflags = O_APPEND | O_WRONLY;
-	unsigned maxlen, len;
-	int msglen;
+	int logfd, result, oflags = O_APPEND | O_WRONLY;
 	char log_file[PATH_MAX];
-	char *logrec;
-	const char *committer;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 	logfd = open(log_file, oflags);
 	if (logfd < 0)
 		return 0;
-	msglen = msg ? strlen(msg) : 0;
-	committer = git_committer_info(0);
-	maxlen = strlen(committer) + msglen + 100;
-	logrec = xmalloc(maxlen);
-	len = sprintf(logrec, "%s %s %s\n",
-		      sha1_to_hex(old_sha1),
-		      sha1_to_hex(new_sha1),
-		      committer);
-	if (msglen)
-		len += copy_msg(logrec + len - 1, msg) - 1;
-	written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
-	free(logrec);
-	if (written != len) {
+	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
+				  git_committer_info(0), msg);
+	if (result) {
 		int save_errno = errno;
 		close(logfd);
 		error("Unable to append to %s", log_file);
-- 
2.2.0.rc2.13.g0786cdb