Re: [patch 2.1.0p1] image.c: more struct stat initialisation fixes

[Mark . Burgess]

Patch checked and applied.
M

On 13 Dec, address@hidden wrote:
> Following my earlier findings in RecursiveImage(), I decided to review
> other uses of struct stat objects in image.c. The following patch
> corrects two definite problems:
>       * in CheckImage(), deststatbuf wasn't initialised before calling
>               CheckCopiedFile()
>       * in PurgeFiles(), the value of statbuf was being used even
>               after a detected and logged lstat() failure
> 
> Furthermore, the patch adds explicit checks of the return status from
> stat() and lstat() wherever they were previously missing. It may be that
> success of these calls was guaranteed by their context, but that's the
> sort of optimisation that makes the code unauditable; better play it
> safe. All failures are logged at cferror: if the code didn't bother
> to check for them, I can only assume they are not meant to happen.
> 
> Side comment: the dstat argument to CopyReg() could be dropped. The
> calling code already relies on this fact by not initialising the
> actual argument before the call, which raises alarm flags at code 
> review time.
> 
> --- orig/src/image.c
> +++ mod/src/image.c
> @@ -463,7 +463,15 @@
>  
>     /* Now check any overrides */
>  
> -   
> CheckCopiedFile(destdir,ip->plus,ip->minus,fixall,ip->uid,ip->gid,&deststatbuf,&sourcestatbuf,NULL,ip->acl_aliases);
> +   if (stat(destdir,&deststatbuf) == -1)
> +      {
> +      snprintf(OUTPUT,bufsize*2,"Can't stat directory %s\n",destdir);
> +      CfLog(cferror,OUTPUT,"stat");
> +      }
> +   else
> +      {
> +      
> CheckCopiedFile(destdir,ip->plus,ip->minus,fixall,ip->uid,ip->gid,&deststatbuf,&sourcestatbuf,NULL,ip->acl_aliases);
> +      }
>     
>     for (dirp = cfreaddir(dirh,ip); dirp != NULL; dirp = cfreaddir(dirh,ip))
>        {
> @@ -583,8 +591,7 @@
>           snprintf(OUTPUT,bufsize*2,"Couldn't stat %s while 
> purging\n",filename);
>           CfLog(cfverbose,OUTPUT,"stat");
>           }
> -      
> -      if (S_ISDIR(statbuf.st_mode))
> +      else if (S_ISDIR(statbuf.st_mode))
>           {
>           struct Tidy tp;
>           struct TidyPattern tpat;
> @@ -722,8 +729,15 @@
>        if (succeed)
>        {
>        ENFORCELINKS = enforcelinks;
> -      lstat(destfile,&deststatbuf);
> -      
> CheckCopiedFile(destfile,ip->plus,ip->minus,fixall,ip->uid,ip->gid,&deststatbuf,&sourcestatbuf,NULL,ip->acl_aliases);
> +      if (lstat(destfile,&deststatbuf) == -1)
> +         {
> +            snprintf(OUTPUT,bufsize*2,"Can't lstat %s\n",destfile);
> +         CfLog(cferror,OUTPUT,"lstat");
> +         }
> +      else
> +         {
> +         
> CheckCopiedFile(destfile,ip->plus,ip->minus,fixall,ip->uid,ip->gid,&deststatbuf,&sourcestatbuf,NULL,ip->acl_aliases);
> +         }
>        }
>        
>        return;
> @@ -826,8 +840,15 @@
>  
>        if (CopyReg(sourcefile,destfile,sourcestatbuf,deststatbuf,ip))
>           {
> -      stat(destfile,&deststatbuf);
> -      
> CheckCopiedFile(destfile,ip->plus,ip->minus,fixall,ip->uid,ip->gid,&deststatbuf,&sourcestatbuf,NULL,ip->acl_aliases);
> +      if (stat(destfile,&deststatbuf) == -1)
> +         {
> +         snprintf(OUTPUT,bufsize*2,"Can't stat %s\n",destfile);
> +         CfLog(cferror,OUTPUT,"stat");
> +         }
> +      else
> +         {
> +         
> CheckCopiedFile(destfile,ip->plus,ip->minus,fixall,ip->uid,ip->gid,&deststatbuf,&sourcestatbuf,NULL,ip->acl_aliases);
> +         }
>        AddMultipleClasses(ip->defines);
>  
>        for (ptr = VAUTODEFINE; ptr != NULL; ptr=ptr->next)
> @@ -966,8 +987,15 @@
>  
>        if (succeed)
>        {
> -      lstat(destfile,&deststatbuf);
> -      
> CheckCopiedFile(destfile,ip->plus,ip->minus,fixall,ip->uid,ip->gid,&deststatbuf,&sourcestatbuf,NULL,ip->acl_aliases);
> +      if (lstat(destfile,&deststatbuf) == -1)
> +         {
> +         snprintf(OUTPUT,bufsize*2,"Can't lstat %s\n",destfile);
> +         CfLog(cferror,OUTPUT,"lstat");
> +         }
> +      else
> +         {
> +         
> CheckCopiedFile(destfile,ip->plus,ip->minus,fixall,ip->uid,ip->gid,&deststatbuf,&sourcestatbuf,NULL,ip->acl_aliases);
> +         }
>        AddMultipleClasses(ip->defines);
>        }
>        }
> @@ -1087,8 +1115,15 @@
>  
>           if (CopyReg(sourcefile,destfile,sourcestatbuf,deststatbuf,ip))
>              {
> -            stat(destfile,&deststatbuf);
> -         
> CheckCopiedFile(destfile,ip->plus,ip->minus,fixall,ip->uid,ip->gid,&deststatbuf,&sourcestatbuf,NULL,ip->acl_aliases);
> +            if (stat(destfile,&deststatbuf) == -1)
> +            {
> +            snprintf(OUTPUT,bufsize*2,"Can't stat %s\n",destfile);
> +            CfLog(cferror,OUTPUT,"stat");
> +            }
> +         else
> +            {
> +            
> CheckCopiedFile(destfile,ip->plus,ip->minus,fixall,ip->uid,ip->gid,&deststatbuf,&sourcestatbuf,NULL,ip->acl_aliases);
> +            }
>  
>           if (VSINGLECOPY != NULL)
>              {
> @@ -1550,7 +1585,12 @@
>        }
>     }
>  
> -stat(new,&dstat);
> +if (stat(new,&dstat) == -1)
> +   {
> +   snprintf(OUTPUT,bufsize*2,"Can't stat new file %s\n",new);
> +   CfLog(cferror,OUTPUT,"stat");
> +   return false;
> +   }
>  
>  if (dstat.st_size != sstat.st_size)
>     {
> 
> 
> 
> 
> _______________________________________________
> Bug-cfengine mailing list
> address@hidden
> http://mail.gnu.org/mailman/listinfo/bug-cfengine



~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Work: +47 22453272            Email:  address@hidden
Fax : +47 22453205            WWW  :  http://www.iu.hio.no/~mark
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~