[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: When to call CheckCopiedFile() ?
From: |
Sergio . Gelato |
Subject: |
Re: When to call CheckCopiedFile() ? |
Date: |
Wed, 17 Dec 2003 19:48:07 +0100 |
User-agent: |
Mutt/1.3.28i |
Earlier I wrote:
> I propose to call CheckCopiedFile() from within CopyReg(), just before
> the rename(new,dest) call.
I since noticed that CopyReg() is also called, without a subsequent
CheckCopiedFile(), from Repository(). I suppose one could simply set
dummy.plus, dummy.minus, dummy.uid and dummy.gid in Repository() to
reasonable values and go ahead, but since this bug is not a show-stopper
I'll just sit and think a while longer.
> 1. potential buffer overflows in forming the name of the backup file,
> particularly when IMAGEBACKUP == 's'. Could be regarded as a
> documentation issue ("Don't use very long destination file names"---
> "Yes, but just how long is too long?"). But why doesn't cfengine
> use strlcat() (or at least strncat()) throughout?
> 2. rename(backup,dest) is attempted unconditionally, even when
> IMAGEBACKUP == 'n' (in which case the backup variable is undefined).
A patch for these two issues follows.
> 3. Wouldn't it be better to perform the sanity checks (size, checksum)
> on the new file first, and only back up/remove the old file if they
> succeed ?
I'll postpone action on that one.
--- orig/src/image.c
+++ mod/src/image.c
@@ -1456,7 +1456,7 @@
{ char backup[bufsize];
char new[bufsize], *linkable;
- int remote = false, silent, backupisdir=false;
+ int remote = false, silent, backupisdir=false, backupok=false;
struct stat s;
#ifdef HAVE_UTIME_H
struct utimbuf timebuf;
@@ -1496,6 +1496,11 @@
remote = true;
}
+if (BufferOverflow(dest,CF_NEW))
+ {
+ printf(" culprit: CopyReg\n");
+ return false;
+ }
strcpy(new,dest);
strcat(new,CF_NEW);
@@ -1538,6 +1543,11 @@
sprintf(stamp, "_%d_%s", CFSTARTTIME, CanonifyName(ctime(&STAMPNOW)));
+ if (BufferOverflow(dest,stamp))
+ {
+ printf(" culprit: CopyReg\n");
+ return false;
+ }
strcpy(backup,dest);
if (IMAGEBACKUP == 's')
@@ -1545,6 +1555,7 @@
strcat(backup,stamp);
}
+ /* rely on prior BufferOverflow() and on strlen(CF_SAVED) < buffer_margin */
strcat(backup,CF_SAVED);
if (IsItemIn(VREPOSLIST,backup))
@@ -1570,6 +1581,7 @@
{
/* ignore */
}
+ backupok = (lstat(backup,&s) != -1); /* Did the rename() succeed?
NFS-safe */
}
else
{
@@ -1596,9 +1608,9 @@
{
snprintf(OUTPUT,bufsize*2,"WARNING: new file %s seems to have been
corrupted in transit (sizes %d and %d), aborting!\n",new, (int) dstat.st_size,
(int) sstat.st_size);
CfLog(cfverbose,OUTPUT,"");
- if (rename(backup,dest) == -1)
+ if (backupok)
{
- /* ignore */
+ rename(backup,dest); /* ignore failure */
}
return false;
}
@@ -1610,8 +1622,9 @@
{
snprintf(OUTPUT,bufsize*2,"WARNING: new file %s seems to have been
corrupted in transit, aborting!\n",new);
CfLog(cfverbose,OUTPUT,"");
- if (rename(backup,dest) == -1)
+ if (backupok)
{
+ rename(backup,dest); /* ignore failure */
}
return false;
}
@@ -1621,7 +1634,10 @@
{
snprintf(OUTPUT,bufsize*2,"Problem: could not install copy file as %s,
directory in the way?\n",dest);
CfLog(cferror,OUTPUT,"rename");
- rename(backup,dest);
+ if (backupok)
+ {
+ rename(backup,dest); /* ignore failure */
+ }
return false;
}