[PATCH] refs.c: add a function to append a reflog entry to a fd
- From
Stefan Beller <sbeller@google.com>
- Date
- Nov 20, 2014, 02:05 UTC
- Message-ID
- <1416449134-12281-1-git-send-email-sbeller@google.com>
- In-Reply-To
- <xmqqvbma4pu3.fsf@gitster.dls.corp.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:
Show 13 quoted lines
>> + 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