From: Junio C Hamano <gitster@pobox•com>
To: Stefan Beller <sbeller@google•com>
Cc: sahlberg@google•com, jrnieder@gmail•com, git@vger•kernel.org
Subject: Re: [PATCH] refs.c: add a function to append a reflog entry to a fd
Date: Wed, 19 Nov 2014 17:22:42 -0800 [thread overview]
Message-ID: <xmqqzjbm4qe5.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1416444142-28042-1-git-send-email-sbeller@google.com> (Stefan Beller's message of "Wed, 19 Nov 2014 16:42:22 -0800")
Stefan Beller <sbeller@google•com> writes:
> Compared to the last send of this patch[1], there was one change in the print
> function. Replaced sprintf by snprintf for security reasons.
Careful. I despise people who blindly think strlcpy() and
snprintf() are good solutions for for security. They are by
themselves not.
Surely, the use of them lets you avoid stomping on pieces of memory
that you did not intend to write to. I'd call that "protecting
other people's data".
But what about your own data? If you are thinking that you are
trying to write "this and that", but by mistake (either by your or
by a careless future change) if you did not allocate enough, you
would stop at "this and th". You may have protected other people's
data, but your result is still broken. If you gave that to others
without checking that you produced an incomplete piece of data, what
guarantee are you getting that you are not creating a security hole
there (imagine you intended to copy "rm -fr ./tmpdir" and by mistake
you stopped at "rm -fr ./" @~@).
> + msglen = msg ? strlen(msg) : 0;
> + maxlen = strlen(committer) + msglen + 100;
> + 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.
So use of snprintf() is not really buying you much here, not in the
current code certainly, but not as a future-proofing measure,
either.
next prev parent reply other threads:[~2014-11-20 1:22 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-20 0:42 [PATCH] refs.c: add a function to append a reflog entry to a fd Stefan Beller
2014-11-20 1:22 ` Junio C Hamano [this message]
2014-11-20 1:34 ` Junio C Hamano
2014-11-20 2:05 ` Stefan Beller
2014-11-20 2:36 ` Jonathan Nieder
2014-11-20 16:57 ` Junio C Hamano
2014-11-20 18:37 ` [PATCH v3] " Stefan Beller
2014-11-20 21:20 ` Jonathan Nieder
2014-11-20 21:24 ` Stefan Beller
2014-11-20 21:31 ` Jonathan Nieder
2014-11-20 21:42 ` Junio C Hamano
2014-11-20 21:58 ` Stefan Beller
2014-11-20 22:05 ` Jonathan Nieder
2014-11-20 22:11 ` Junio C Hamano
2014-11-20 21:59 ` [PATCH v4] " Stefan Beller
2014-11-20 22:11 ` Jonathan Nieder
2014-11-20 22:29 ` Junio C Hamano
2014-11-20 1:42 ` [PATCH] " 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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqzjbm4qe5.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jrnieder@gmail$(echo .)com \
--cc=sahlberg@google$(echo .)com \
--cc=sbeller@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox