Jeff King <peff@xxxxxxxx> writes: > On Sun, Jan 15, 2023 at 12:03:39AM -0800, Junio C Hamano wrote: > >> The "subject_prefix" member of "struct revision" usually is always >> set to a borrowed string (either a string literal like "PATCH" that >> appear in the program text as a hardcoded default, or the value of >> "format.subjectprefix") and is never freed when the containing >> revision structure is released. The "-v <num>" codepath however >> violates this rule and stores a pointer to an allocated string to >> this member, relinquishing the responsibility to free it when it is >> done using the revision structure, leading to a small one-time leak. >> >> Instead, keep track of the string it allocates to let the revision >> structure borrow, and clean it up when it is done. > > FWIW, this looks obviously correct to me. > > The word "unleak" in the subject made me think about UNLEAK(), so this > is a small tangent. This is exactly the kind of case that I designed > UNLEAK() for, because the solution really is "while you are assigning to > X, also keep a copy of the pointer in Y to be freed later". Yup. I was originally planning to use UNLEAK(), but it felt ugly to UNLEAK(rev.subject_prefix), as it stores borrowed pointer sometimes and owned pointer some other times, which is the exact reason why I started looking for a clean way to plug this leak. So I ended up with declaring that the member should only store a borrowed pointer. > And UNLEAK() is just "keep a copy of the pointer in Y to know that we > _could_ free it later". And of course "do nothing if we are not > leak-detecting". But since we seem to be moving away from UNLEAK(), and > since it would not even save any lines here, I'm perfectly happy with > this solution. The first sentence needs to be rephrased, as it does not make much sense to have something usually be X and always be X at the same time (I'd just remove "always" from there). Thanks.
