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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html