Re: [PATCH v2 20/21] refs: add LMDB refs backend

From: Jeff King <peff@peff.net>
Date: Jan 14, 2016, 20:52 UTC
Message-ID: <20160114205259.GA10440@sigill.intra.peff.net>
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:

Show 23 quoted lines

> +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?).

Show 19 quoted lines

> +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