Re: Patch for bin/54446

On Nov 26, 2008, at 4:20 AM, insomniac wrote:

> On Tue, 25 Nov 2008 19:33:37 -0800
> Xin LI <delphij_at_delphij.net> wrote:
>
>> I have made a small change: use malloc() here and use strlcpy().
>> Other parts looks just fine.
>>
>> (BTW I think we need to cc portmgr_at_ for approval)
>
> I  merged your fix with Garrett Cooper's one (I forgot to free()  
> before
> a return) and also mailed portmgr_at_ for approval. The new patch is
> located at the same link:
>
> http://insomniac.slackware.it/plist.c.diff
>
> Moreover, I will fix style(9) in a second step.
>
> Best regards,
> -- 
> Andrea Barberio

Xin Li brought up a really good point (that I remembered when I first  
looked at the code, but forgot to mention later):

calloc is nothing more than a malloc + bzero on many OSes (FreeBSD has  
moved away from this methodology), so using calloc sparingly is for  
the best. It really doesn't buy you much here anyhow since you're  
overwriting everything with a copied string anyhow...

As for the potential security issue I mentioned earlier, it could be  
definitely be done if someone has the knowledge and was running with  
similar privs or had write access to the parent / child(ren) symlink.  
One of the ways of solving this issue may be to use flock(2) *shrugs*,  
as it would block other callers from modifying the file or the symlink  
(our concern would be symlinks I would think).

This kind of race condition situation is part of the driving force  
between using fstat vs lstat vs stat.

HTH,
-Garrett