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

[David Turner <dturner@twopensource.com>]

On Thu, 2016-01-14 at 15:52 -0500, Jeff King wrote:
> 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?).

Did both.

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

OK.

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

OK, I went ahead and did all of these.

Thanks for the review.