volser-dump-validate-input-20060417

This patch adds code to perform return code error checking for all calls
to the ReadXyz() routines.  If the rx connection was lost or if the dump
format being parsed was incorrect there was an opportunity for data to
the processed after an error had already been detected.  In the best case
this would have simply resulted in additional work being performed when
it was not necessary but there was also the potential for modifying return
values which would later be used as input to later operations.  Better
to simply catch the error when it initially occurs and be done with it.

The ReadString() function had the potential to write into one byte prior
to the start of the input array.

ProcessIndex() would not properly close Fdhandle nor the StreamHandle
if an error was detected.

RestoreVolume() had a problem with the processing of incremental dumps.
Originally AFS supported a concept of incrementals dumps which when
restored would simply add their contents to the existing volume.  One
of the distinctions between the incremental and full dumps is that full
dump processing is responsible for removing any vnode entries from the
volume that are not present in the dump file headers.  When the incremental
dump support was removed, the RestoreVolume() function was broken.  If
a dump file contains multiple Dump Headers, then only the vnode entries
in the first Dump Header would be marked as being restored.  Any vnode
entries restored from subsequent Dump Headers would be deleted when the
RestoreVolume() function reached the end.  By removing the assignment
of 'tdelo = -1' within the loop that processes the Dump Headers, this
bug is resolved.  Although RestoreVolume is only called with the
'incremental' parameter == 0 at the current time, the check for non-zero
values is restored and the "remove unrestored vnodes" functionality is
disabled when a non-zero value is provided.

The ReadVnodes() function did not perform error checking.

diff --git a/src/volser/dumpstuff.c b/src/volser/dumpstuff.c
index fc16c527931de79413f58daeb9b0d12d3d6095ed..73765d028137293c83de6247018c4bfffd076049 100644 (file)
--- a/src/volser/dumpstuff.c
+++ b/src/volser/dumpstuff.c
@@ -212,9 +212,13 @@ ReadShort(register struct iod *iodp, register unsigned short *sp)
 {
     register b1, b0;
     b1 = iod_getc(iodp);
+    if (b1 == EOF)
+       return 0;
     b0 = iod_getc(iodp);
+    if (b0 == EOF)
+       return 0;
     *sp = (b1 << 8) | b0;
-    return b0 != EOF;
+    return 1;
 }
 
 static int
@@ -222,24 +226,38 @@ ReadInt32(register struct iod *iodp, afs_uint32 * lp)
 {
     afs_uint32 register b3, b2, b1, b0;
     b3 = iod_getc(iodp);
+    if (b3 == EOF)
+       return 0;
     b2 = iod_getc(iodp);
+    if (b2 == EOF)
+       return 0;
     b1 = iod_getc(iodp);
+    if (b1 == EOF)
+       return 0;
     b0 = iod_getc(iodp);
+    if (b0 == EOF)
+       return 0;
     *lp = (((((b3 << 8) | b2) << 8) | b1) << 8) | b0;
-    return b0 != EOF;
+    return 1;
 }
 
 static void
 ReadString(register struct iod *iodp, register char *to, register int maxa)
 {
     register int c;
+    int first = 1;
+
+    *to = '\0';
+    if (maxa == 0)
+       return;
+
     while (maxa--) {
-       if ((*to++ = iod_getc(iodp)) == 0)
+       if ((*to++ = c = iod_getc(iodp)) == 0 || c == EOF)
            break;
     }
     if (to[-1]) {
        while ((c = iod_getc(iodp)) && c != EOF);
-       to[-1] = 0;
+       to[-1] = '\0';
     }
 }
 
@@ -962,8 +980,11 @@ ProcessIndex(Volume * vp, VnodeClass class, afs_int32 ** Bufp, int *sizep,
             vcp->diskSize ? 0 : size - vcp->diskSize) >> vcp->logSize;
        if (nVnodes > 0) {
            Buf = (afs_int32 *) malloc(nVnodes * sizeof(afs_int32));
-           if (Buf == NULL)
+           if (Buf == NULL) {
+               STREAM_CLOSE(afile);
+               FDH_CLOSE(fdP);
                return 1;
+           }
            memset((char *)Buf, 0, nVnodes * sizeof(afs_int32));
            STREAM_SEEK(afile, offset = vcp->diskSize, 0);
            while (1) {
@@ -998,8 +1019,8 @@ RestoreVolume(register struct rx_call *call, Volume * avp, int incremental,
     register Volume *vp;
     struct iod iod;
     register struct iod *iodp = &iod;
-    afs_int32 *b1 = 0, *b2 = 0;
-    int s1 = 0, s2 = 0, delo = 0, tdelo;
+    afs_int32 *b1 = NULL, *b2 = NULL;
+    int s1 = 0, s2 = 0, delo = incremental, tdelo;
     int tag;
 
     iod_Init(iodp, call);
@@ -1016,7 +1037,8 @@ RestoreVolume(register struct rx_call *call, Volume * avp, int incremental,
     if (ReadVolumeHeader(iodp, &vol) == VOLSERREAD_DUMPERROR)
        return VOLSERREAD_DUMPERROR;
 
-    delo = ProcessIndex(vp, vLarge, &b1, &s1, 0);
+    if (!delo)
+       delo = ProcessIndex(vp, vLarge, &b1, &s1, 0);
     if (!delo)
        delo = ProcessIndex(vp, vSmall, &b2, &s2, 0);
     if (delo) {
@@ -1024,7 +1046,8 @@ RestoreVolume(register struct rx_call *call, Volume * avp, int incremental,
            free((char *)b1);
        if (b2)
            free((char *)b2);
-       b1 = b2 = 0;
+       b1 = b2 = NULL;
+       s1 = s2 = 0;
     }
 
     strncpy(vol.name, cookie->name, VOLSER_OLDMAXVOLNAME);
@@ -1042,11 +1065,11 @@ RestoreVolume(register struct rx_call *call, Volume * avp, int incremental,
        tag = iod_getc(iodp);
        if (tag != D_VOLUMEHEADER)
            break;
+
        if (ReadVolumeHeader(iodp, &vol) == VOLSERREAD_DUMPERROR) {
            error = VOLSERREAD_DUMPERROR;
            goto out;
        }
-       tdelo = -1;
     }
     if (tag != D_DUMPEND || !ReadInt32(iodp, &endMagic)
        || endMagic != DUMPENDMAGIC) {
@@ -1063,8 +1086,13 @@ RestoreVolume(register struct rx_call *call, Volume * avp, int incremental,
     }
 
     if (!delo) {
-       ProcessIndex(vp, vLarge, &b1, &s1, 1);
-       ProcessIndex(vp, vSmall, &b2, &s2, 1);
+       delo = ProcessIndex(vp, vLarge, &b1, &s1, 1);
+       if (!delo)
+           ProcessIndex(vp, vSmall, &b2, &s2, 1);
+       if (delo) {
+           error = VOLSERREAD_DUMPERROR;
+           goto clean;
+       }
     }
 
   clean:
@@ -1111,7 +1139,8 @@ ReadVnodes(register struct iod *iodp, Volume * vp, int incremental,
        if (!ReadInt32(iodp, (afs_uint32 *) & vnodeNumber))
            break;
 
-       ReadInt32(iodp, &vnode->uniquifier);
+       if (!ReadInt32(iodp, &vnode->uniquifier))
+           return VOLSERREAD_DUMPERROR;
        while ((tag = iod_getc(iodp)) > D_MAX && tag != EOF) {
            haveStuff = 1;
            switch (tag) {
@@ -1121,36 +1150,45 @@ ReadVnodes(register struct iod *iodp, Volume * vp, int incremental,
            case 'l':
                {
                    unsigned short tlc;
-                   ReadShort(iodp, &tlc);
+                   if (!ReadShort(iodp, &tlc))
+                       return VOLSERREAD_DUMPERROR;
                    vnode->linkCount = (signed int)tlc;
                }
                break;
            case 'v':
-               ReadInt32(iodp, &vnode->dataVersion);
+               if (!ReadInt32(iodp, &vnode->dataVersion))
+                   return VOLSERREAD_DUMPERROR;
                break;
            case 'm':
-               ReadInt32(iodp, &vnode->unixModifyTime);
+               if (!ReadInt32(iodp, &vnode->unixModifyTime))
+                   return VOLSERREAD_DUMPERROR;
                break;
            case 's':
-               ReadInt32(iodp, &vnode->serverModifyTime);
+               if (!ReadInt32(iodp, &vnode->serverModifyTime))
+                   return VOLSERREAD_DUMPERROR;
                break;
            case 'a':
-               ReadInt32(iodp, &vnode->author);
+               if (!ReadInt32(iodp, &vnode->author))
+                   return VOLSERREAD_DUMPERROR;
                break;
            case 'o':
-               ReadInt32(iodp, &vnode->owner);
+               if (!ReadInt32(iodp, &vnode->owner))
+                   return VOLSERREAD_DUMPERROR;
                break;
            case 'g':
-               ReadInt32(iodp, (afs_uint32 *) & vnode->group);
+               if (!ReadInt32(iodp, (afs_uint32 *) & vnode->group))
+                   return VOLSERREAD_DUMPERROR;
                break;
            case 'b':{
                    unsigned short modeBits;
-                   ReadShort(iodp, &modeBits);
+                   if (!ReadShort(iodp, &modeBits))
+                       return VOLSERREAD_DUMPERROR;
                    vnode->modeBits = (unsigned int)modeBits;
                    break;
                }
            case 'p':
-               ReadInt32(iodp, &vnode->parent);
+               if (!ReadInt32(iodp, &vnode->parent))
+                   return VOLSERREAD_DUMPERROR;
                break;
            case 'A':
                ReadByteString(iodp, (byte *) VVnodeDiskACL(vnode),
@@ -1250,7 +1288,6 @@ ReadVnodes(register struct iod *iodp, Volume * vp, int incremental,
     }
     iod_ungetc(iodp, tag);
 
-
     return 0;
 }