Jeff King <peff@xxxxxxxx> writes: > There is one more, I think: if you _do_ free the allocated string to > avoid the leak you mention, then some other code which was relying on > the lifetime of that string to be effectively infinite will now have a > user-after-free. Ah, yes, you're right. I completely forgot about that shallow copy. > A few other things to note, looking at this code: > > - isn't kvi->path in the same boat? We do not duplicate it at all, so > it seems like the shallow copy made in the configset could cause a > user-after-free. > > - the "fix" I showed above hits your point 2: now we are making a lot > more copies of that string. I will note that we're already making a > lot of copies of the kvi struct in the first place, so unless you > have really long pathnames, it probably isn't a big difference. > > But it possibly could make sense to have the configset own a single > duplicate string, and then let the kvi structs it holds point to > that string. But IMHO all of this should be details of the configset > code, and the main config-iteration code should not have to worry > about this at all. I.e., I think kvi_from_source() should not be > duplicating anything in the first place. Thanks for a detailed write-up.
