FUSE Readlink Fix / Datastream Traversal Fixes / Temporary Removal of…

… Automatic DAL Checks (#233)

* Changes to streamwalker + marfs-rman to account for possible missing reference file at end of datastream and correction to blatant NULL pointer deref in marfs-rman in the case of a missing reference file FTAG value

* Correction to previous and downgrade of posixmdal_openscanner ENOENT failures to informational only

* Updated fuse implementation to properly reflect fuse readlink interface

* Minor adjustments to datastream MDAL handle close logic

* Temporarily disabled DAL checking during initialization of marfs contexts ( by default ) due to performance implications.  Corrected possible edge case associated with improperly sizing a string buffer when failing to retrieve FTAG xattrs.

src/api/marfs.c

@@ -238,8 +238,14 @@ marfs_ctxt marfs_init( const char* configpath, marfs_interface type, pthread_mut
       return NULL;
    }
    // verify our config
-   int verifyflags = CFG_MDALCHECK | CFG_DALCHECK;
-   if ( getuid() == 0 ) { verifyflags |= CFG_RECURSE; } // only attempt to recurse if we are running as root ( guarantess sub-NS access )
+   int verifyflags = CFG_MDALCHECK;
+   /**
+   * TODO Need to handle automatic config verification in a more efficient way
+   *   Full DAL scatter check is prohibitively intensive for startup of most programs
+   *   Even full recursion of all NS paths *may* be too intensive for some cases
+   */
+   //int verifyflags = CFG_MDALCHECK | CFG_DALCHECK;
+   //if ( getuid() == 0 ) { verifyflags |= CFG_RECURSE; } // only attempt to recurse if we are running as root ( guarantess sub-NS access )
    if ( config_verify( ctxt->config, ".", verifyflags ) ) {
       LOG( LOG_ERR, "Encountered uncorrected errors with the MarFS config\n" );
       config_term( ctxt->config );

src/datastream/datastream.c

@@ -257,6 +257,7 @@ void freestream(DATASTREAM stream) {
          if (stream->files[curfile].metahandle && ms->mdal->close(stream->files[curfile].metahandle)) {
             LOG(LOG_WARNING, "Failed to close meta handle for file %zu\n", curfile);
          }
+         stream->files[curfile].metahandle = NULL; // likely unnecessary, but just in case
       }
       // free the file list itself
       free(stream->files);
@@ -1338,6 +1339,7 @@ int close_current_obj(DATASTREAM stream, FTAG* curftag, MDAL_CTXT mdalctxt) {
          struct stat stval = {0};
          if ( mdal->fstat( rhandle, &(stval) ) ) {
             LOG( LOG_ERR, "Failed to stat newly created rebuild marker\n" );
+            mdal->close(rhandle);
             rtag_free( &(rtag) );
             if (releasectxt) {
                mdal->destroyctxt(mdalctxt);

src/datastream/datastream.h

@@ -95,10 +95,10 @@ typedef struct datastream_struct {
    size_t      filealloc;
    RECOVERY_FINFO finfo;
    // Temporary Buffers
-   char* ftagstr;
+   char*       ftagstr;
    size_t      ftagstrsize;
-   char* finfostr;
-   size_t finfostrlen;
+   char*       finfostr;
+   size_t      finfostrlen;
 }*DATASTREAM;
 
 /**

src/datastream/streamwalker.c

@@ -591,6 +591,13 @@ int populate_tags(marfs_config* config, marfs_position* pathpos, const char* pat
       free(modpath);
    }
    if (handle == NULL) {
+      if ( errno == ENOENT ) {
+         // handle ENOENT slightly differently, as this is a semi-expected case
+         printf(OUTPREFX "WARNING: Failed to open target %s file: \"%s\" (%s)\n",
+            (rpath) ? "ref" : "user", (rpath) ? rpath : path, strerror(errno));
+         config_abandonposition( &oppos );
+         return 1;
+      }
       printf(OUTPREFX "ERROR: Failed to open target %s file: \"%s\" (%s)\n",
          (rpath) ? "ref" : "user", (rpath) ? rpath : path, strerror(errno));
       config_abandonposition( &oppos );
@@ -1234,6 +1241,9 @@ int bounds_command(marfs_config* config, walkerstate* state, char* args) {
          // retrieve the FTAG of the new target
          walkerinfo info = {0};
          retval = populate_tags(config, &(state->pos), NULL, newrpath, 0, &info);
+         if ( retval == 1  &&  (state->ftag.state & FTAG_DATASTATE) == FTAG_FIN ) { // ENOENT after a FINALIZED file is a special case
+            break;
+         }
          if ( !retval ) {
             retval = update_state(&info, state, 0);
          }
@@ -1266,9 +1276,9 @@ int bounds_command(marfs_config* config, walkerstate* state, char* args) {
       }
       // retrieve the FTAG of the new target
       walkerinfo info = {0};
-      retval = populate_tags(config, &(state->pos), NULL, newrpath, 0, &info);
-      if ( !retval ) {
-         retval = update_state(&info, state, 0);
+      int tmpretval = populate_tags(config, &(state->pos), NULL, newrpath, 0, &info);
+      if ( !tmpretval ) {
+         tmpretval = update_state(&info, state, 0);
       }
       free(newrpath);
       if ( errorflag ) { return -1; }

src/fuse/fuse.c

@@ -73,6 +73,7 @@ GNU licenses can be found at http://www.gnu.org/licenses/.
 #include <unistd.h>
 #include <dirent.h>
 #include <errno.h>
+#include <limits.h>
 
 #include "change_user.h"
 #include "api/marfs.h"
@@ -747,10 +748,18 @@ int fuse_readlink(const char *path, char *buf, size_t size)
     LOG( LOG_ERR, "%s: %s\n", path, strerror(errno) );
     return (errno) ? -errno : -ENOMSG;
   }
+  // sanity check return values and null-terminate on the behalf of fuse because it just can't be bothered to define its readlink interface sensibly
+  if ( ret < size ) { *(buf + ret) = '\0'; ret = 0; } // NULL terminate the string, and indicate success
+  else {
+    if ( size > 0 ) { // silly check to try to properly null-terminate, even if we're called to fill a zero-length buffer
+      *(buf + (size - 1)) = '\0';
+    }
+    if ( ret > INT_MAX ) { ret = INT_MAX; }
+  }
 
   exit_user(&u_ctxt);
 
-  return 0;
+  return (int)ret;
 }
 
 int fuse_release(const char *path, struct fuse_file_info *ffi)

src/mdal/posix_mdal.c

@@ -1814,7 +1814,12 @@ MDAL_SCANNER posixmdal_openscanner( MDAL_CTXT ctxt, const char* rpath ) {
    // open the target
    int dfd = openat( pctxt->refd, rpath, O_RDONLY );
    if ( dfd < 0 ) {
-      LOG( LOG_ERR, "Failed to open the target path: \"%s\"\n", rpath );
+      if ( errno == ENOENT ) { // very common case, downgrade to informational only
+         LOG( LOG_INFO, "Failed to open the target path: \"%s\" ( %s )\n", rpath, strerror(errno) );
+      }
+      else {
+         LOG( LOG_ERR, "Failed to open the target path: \"%s\" ( %s )\n", rpath, strerror(errno) );
+      }
       return NULL;
    }
    // verify the target is a dir

src/rsrc_mgr/resourceprocessing.c

@@ -915,6 +915,32 @@ void destroystreamwalker( streamwalker walker ) {
    }
 }
 
+
+int searchoperations( opinfo** opchain, operation_type type, FTAG* ftag, opinfo** optgt ) {
+   opinfo* prevop = NULL;
+   if ( opchain  &&  *opchain ) {
+      // check for any existing ops of this type in the chain
+      opinfo* parseop = *opchain;
+      while ( parseop ) {
+         // for most ops, matching on type is sufficent to reuse the same op tgt
+         // For object deletions, repacks, and rebuilds, we need to check that the new tgt is in the same 'chain'
+         if ( parseop->type == type  &&
+              ( type != MARFS_DELETE_OBJ_OP  ||  ftag->objno == (parseop->ftag.objno + parseop->count) )  &&
+              ( type != MARFS_REPACK_OP  ||  ftag->fileno == (parseop->ftag.fileno + parseop->count) )  &&
+              ( type != MARFS_REBUILD_OP  ||  ftag->objno == (parseop->ftag.objno + parseop->count) )
+            ) {
+            *optgt = parseop;
+            return 0;
+         }
+         prevop = parseop;
+         parseop = parseop->next;
+      }
+   }
+   *optgt = prevop; // return a reference to the tail of the operation chain
+   return -1;
+}
+
+
 int process_getfileinfo( const char* reftgt, char getxattrs, streamwalker walker, char* filestate ) {
    MDAL mdal = walker->pos.ns->prepo->metascheme.mdal;
    if ( getxattrs ) {
@@ -980,7 +1006,7 @@ int process_getfileinfo( const char* reftgt, char getxattrs, streamwalker walker
       // retrieve FTAG of the current file
       getres = mdal->fgetxattr( handle, 1, FTAG_NAME, walker->ftagstr, walker->ftagstralloc - 1 );
       // check for overflow
-      if ( getres >= walker->ftagstralloc ) {
+      if ( getres > 0  &&  getres >= walker->ftagstralloc ) {
          // double our allocated string length
          char* newstr = malloc( sizeof(char) * (getres + 1) );
          if ( newstr == NULL ) {
@@ -1066,24 +1092,11 @@ int process_getfileinfo( const char* reftgt, char getxattrs, streamwalker walker
 }
 
 int process_identifyoperation( opinfo** opchain, operation_type type, FTAG* ftag, opinfo** optgt ) {
+   // check for any existing ops of this type in the chain
    opinfo* prevop = NULL;
-   if ( opchain  &&  *opchain ) {
-      // check for any existing ops of this type in the chain
-      opinfo* parseop = *opchain;
-      while ( parseop ) {
-         // for most ops, matching on type is sufficent to reuse the same op tgt
-         // For object deletions, repacks, and rebuilds, we need to check that the new tgt is in the same 'chain'
-         if ( parseop->type == type  &&
-              ( type != MARFS_DELETE_OBJ_OP  ||  ftag->objno == (parseop->ftag.objno + parseop->count) )  &&
-              ( type != MARFS_REPACK_OP  ||  ftag->fileno == (parseop->ftag.fileno + parseop->count) )  &&
-              ( type != MARFS_REBUILD_OP  ||  ftag->objno == (parseop->ftag.objno + parseop->count) )
-            ) {
-            *optgt = parseop;
-            return 0;
-         }
-         prevop = parseop;
-         parseop = parseop->next;
-      }
+   if ( searchoperations( opchain, type, ftag, &(prevop) ) == 0 ) {
+      *optgt = prevop;
+      return 0;
    }
    // allocate a new operation struct
    opinfo* newop = malloc( sizeof( struct opinfo_struct ) );
@@ -1994,13 +2007,12 @@ int process_iteratestreamwalker( streamwalker* swalker, opinfo** gcops, opinfo**
                free( reftgt );
                return -1;
             }
-            delref_info* delrefinf = NULL;
+            delref_info* delrefinf = optgt->extendedinfo;
             delrefinf->eos = 1; // ALWAYS set this as EndOfStream
             if ( optgt->count ) {
                // update existing op
                optgt->count++;
                optgt->count += walker->gctag.refcnt;
-               delrefinf = optgt->extendedinfo;
                // sanity check
                if ( delrefinf->prev_active_index != walker->activeindex ) {
                   LOG( LOG_ERR, "Active delref op active index (%zu) does not match current val (%zu)\n",
@@ -2014,7 +2026,6 @@ int process_iteratestreamwalker( streamwalker* swalker, opinfo** gcops, opinfo**
                optgt->ftag.fileno = walker->ftag.fileno; // be SURE that this is not a dummy op, targeting fileno zero
                optgt->count = 1;
                optgt->count += walker->gctag.refcnt;
-               delrefinf = optgt->extendedinfo;
                delrefinf->prev_active_index = walker->activeindex;
             }
             walker->report.delfiles++;
@@ -2033,11 +2044,29 @@ int process_iteratestreamwalker( streamwalker* swalker, opinfo** gcops, opinfo**
          if ( walker->fileno == 0 ) {
             // can't decrement beyond the beginning of the datastream
             LOG( LOG_ERR, "Initial reference target does not exist: \"%s\"\n", reftgt );
+            free( reftgt );
             return -1;
          }
          if ( walker->fileno == walker->ftag.fileno ) {
             // looks like we already pulled xattrs from the previous file, and must not have found a GCTAG
+            if ( (walker->ftag.state & FTAG_DATASTATE) == FTAG_FIN ) {
+               // special case, writer has yet to / failed to create the next reference file
+               LOG( LOG_WARNING, "Datastream break ( assumed EOS ) detected at file number %zu: \"%s\"\n", walker->fileno, reftgt );
+               free( reftgt );
+               // modify any active GC ref-del op to properly include EOS value
+               if ( walker->gcthresh ) {
+                  opinfo* optgt = NULL;
+                  if ( searchoperations( &(walker->gcops), MARFS_DELETE_REF_OP, &(tmptag), &(optgt) ) == 0 ) {
+                     LOG( LOG_INFO, "Detected outstanding reference deletion op is being set to assume EOS\n" );
+                     delref_info* delrefinf = optgt->extendedinfo;
+                     delrefinf->eos = 1; // set to assume EndOfStream
+                  }
+               }
+               walker->ftag.endofstream = 1; // set previous ftag value to reflect assumed EOS
+               break;
+            }
             LOG( LOG_ERR, "Datastream break detected at file number %zu: \"%s\"\n", walker->fileno, reftgt );
+            free( reftgt );
             return -1;
          }
          // generate the rpath of the previous file
@@ -2069,13 +2098,12 @@ int process_iteratestreamwalker( streamwalker* swalker, opinfo** gcops, opinfo**
                   free( reftgt );
                   return -1;
                }
-               delref_info* delrefinf = NULL;
+               delref_info* delrefinf = optgt->extendedinfo;
                delrefinf->eos = 1; // ALWAYS set this as EndOfStream
                if ( optgt->count ) {
                   // update existing op
                   optgt->count++;
                   optgt->count += walker->gctag.refcnt;
-                  delrefinf = optgt->extendedinfo;
                   // sanity check
                   if ( delrefinf->prev_active_index != walker->activeindex ) {
                      LOG( LOG_ERR, "Active delref op active index (%zu) does not match current val (%zu)\n",
@@ -2089,7 +2117,6 @@ int process_iteratestreamwalker( streamwalker* swalker, opinfo** gcops, opinfo**
                   optgt->ftag.fileno = walker->ftag.fileno; // be SURE that this is not a dummy op, targeting fileno zero
                   optgt->count = 1;
                   optgt->count += walker->gctag.refcnt;
-                  delrefinf = optgt->extendedinfo;
                   delrefinf->prev_active_index = walker->activeindex;
                }
                walker->report.delfiles++;