From: Jeff King <peff@peff.net>
To: David Turner <dturner@twopensource.com>
Cc: git@vger.kernel.org, mhagger@alum.mit.edu
Subject: Re: [PATCH v2 20/21] refs: add LMDB refs backend
Date: Thu, 14 Jan 2016 15:52:59 -0500 [thread overview]
Message-ID: <20160114205259.GA10440@sigill.intra.peff.net> (raw)
In-Reply-To: <1452561740-8668-21-git-send-email-dturner@twopensource.com>
On Mon, Jan 11, 2016 at 08:22:19PM -0500, David Turner wrote:
> +static int rename_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
> + const char *email, unsigned long timestamp, int tz,
> + const char *message, void *cb_data)
> +{
> +
> + const char *newrefname = cb_data;
> + MDB_val key, new_key, val;
> +
> + assert(transaction.cursor);
> +
> + if (mdb_cursor_get_or_die(transaction.cursor, &key, &val, MDB_GET_CURRENT))
> + die("renaming ref: mdb_cursor_get failed to get current");
> +
> + new_key.mv_size = strlen(newrefname) + 5 + 1 + 8;
> + new_key.mv_data = xmalloc(new_key.mv_size);
> + strcpy(new_key.mv_data, "logs/");
> + strcpy((char *)new_key.mv_data + 5, newrefname);
> + memcpy((char *)new_key.mv_data + new_key.mv_size - 8,
> + (const char *)key.mv_data + key.mv_size - 8, 8);
> + mdb_put_or_die(&transaction, &new_key, &val, 0);
> + mdb_cursor_del_or_die(transaction.cursor, 0);
> + free(new_key.mv_data);
> + return 0;
When you re-roll, do you mind avoiding strcpy here? I know that your
malloc is big enough, but:
1. Avoiding strcpy makes auditing easier.
2. We can probably come up with a solution that avoids the magic
numbers, making it more pleasant to read.
3. Manual computation plus a strcpy can be vulnerable to integer
overflows in the size (I didn't check the types on MDB_val to see
if that is feasible or not, but again, it's nice to avoid for audit
purposes).
Since we free the memory immediately-ish, I think using a strbuf would
be a good fit. Something like:
struct strbuf path = STRBUF_INIT;
...
strbuf_addf(&path, "logs/%s", newrefname);
strbuf_add(&path, (const char *)key.mv_data + key.mv_size - 8, 8);
new_key.mv_size = path.len;
new_key.mv_data = path.buf;
... mdb_put, etc ...
strbuf_release(&path);
(I hope I'm reading the 8-byte thing right; should we also be asserting
that key.mv_size >= 8?).
> +static int lmdb_for_each_reflog_ent_order(const char *refname,
> + each_reflog_ent_fn fn,
> + void *cb_data, int reverse)
> +{
> + MDB_val key, val;
> + char *search_key;
> + char *log_path;
> + int len;
> + MDB_cursor *cursor;
> + int ret = 0;
> + struct strbuf sb = STRBUF_INIT;
> + enum MDB_cursor_op direction = reverse ? MDB_PREV : MDB_NEXT;
> + uint64_t zero = 0ULL;
> +
> + len = strlen(refname) + 6;
> + log_path = xmalloc(len);
> + search_key = xmalloc(len + 1);
> + sprintf(log_path, "logs/%s", refname);
> + strcpy(search_key, log_path);
Ditto here (and for sprintf, too). You can do these with xstrfmt:
log_path = xstrfmt("logs/%s", refname);
len = strlen(log_path); /* or use a strbuf to avoid the extra strlen */
The search_key one looks like an extra off-by-one, but the extra byte
gets used below. So maybe:
/* \0 may be rewritten as \1 for reverse search below */
search_key = xstrfmt("%s\0", log_path);
though I think:
if (reverse) {
/* explanation ... */
search_key = xstrfmt("%s\1", log_path);
} else {
search_key = xstrdup(log_path);
}
might be clearer to a reader. There are a few other sprintfs and
strcpys, but I think they can all use similar techniques.
-Peff
next prev parent reply other threads:[~2016-01-14 20:53 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-12 1:21 [PATCH v2 00/21] refs backend reroll David Turner
2016-01-12 1:22 ` [PATCH v2 01/21] refs: add a backend method structure with transaction functions David Turner
2016-01-12 1:22 ` [PATCH v2 02/21] refs: add methods for misc ref operations David Turner
2016-01-12 1:22 ` [PATCH v2 03/21] refs: add methods for the ref iterators David Turner
2016-01-12 1:22 ` [PATCH v2 04/21] refs: add do_for_each_per_worktree_ref David Turner
2016-01-12 1:22 ` [PATCH v2 05/21] refs: add methods for reflog David Turner
2016-01-12 1:22 ` [PATCH v2 06/21] refs: add method for initial ref transaction commit David Turner
2016-01-12 1:22 ` [PATCH v2 07/21] refs: add method for delete_refs David Turner
2016-01-12 1:22 ` [PATCH v2 08/21] refs: add methods to init refs db David Turner
2016-01-12 1:22 ` [PATCH v2 09/21] refs: add method to rename refs David Turner
2016-01-12 1:22 ` [PATCH v2 10/21] refs: make lock generic David Turner
2016-01-12 1:22 ` [PATCH v2 11/21] refs: move duplicate check to common code David Turner
2016-01-12 1:22 ` [PATCH v2 12/21] refs: allow log-only updates David Turner
2016-01-12 1:22 ` [PATCH v2 13/21] refs: resolve symbolic refs first David Turner
2016-01-13 0:19 ` Junio C Hamano
2016-01-13 16:33 ` David Turner
2016-01-12 1:22 ` [PATCH v2 14/21] refs: always handle non-normal refs in files backend David Turner
2016-01-13 0:19 ` Junio C Hamano
2016-01-13 16:34 ` David Turner
2016-01-12 1:22 ` [PATCH v2 15/21] init: allow alternate backends to be set for new repos David Turner
2016-01-12 1:22 ` [PATCH v2 16/21] refs: check submodules ref storage config David Turner
2016-01-13 0:19 ` Junio C Hamano
2016-01-13 16:32 ` David Turner
2016-01-13 17:02 ` Junio C Hamano
2016-01-12 1:22 ` [PATCH v2 17/21] clone: use child_process for recursive checkouts David Turner
2016-01-12 1:22 ` [PATCH v2 18/21] refs: allow ref backend to be set for clone David Turner
2016-01-12 1:22 ` [PATCH v2 19/21] svn: learn ref-storage argument David Turner
2016-01-12 1:22 ` [PATCH v2 20/21] refs: add LMDB refs backend David Turner
2016-01-14 20:52 ` Jeff King [this message]
2016-01-14 22:30 ` David Turner
2016-01-12 1:22 ` [PATCH v2 21/21] refs: tests for lmdb backend David Turner
2016-01-13 0:22 ` [PATCH v2 00/21] refs backend reroll Junio C Hamano
2016-01-14 9:26 ` Jeff King
2016-01-14 16:25 ` David Turner
2016-01-14 20:12 ` Jeff King
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=20160114205259.GA10440@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=dturner@twopensource.com \
--cc=git@vger.kernel.org \
--cc=mhagger@alum.mit.edu \
/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).