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