Merge pull request #1905 from facebook/devnull

fix #1904

.travis.yml

@@ -34,7 +34,8 @@ matrix:
 
     - name: make test (complete)
       script:
-        - make test
+        # DEVNULLRIGHTS : will request sudo rights to test permissions on /dev/null
+        - DEVNULLRIGHTS=test make test
 
     - name: gcc-6 + gcc-7 compilation
       script:

programs/fileio.c

@@ -502,7 +502,7 @@ static int FIO_remove(const char* path)
 #if defined(_WIN32) || defined(WIN32)
     /* windows doesn't allow remove read-only files,
      * so try to make it writable first */
-    chmod(path, _S_IWRITE);
+    UTIL_chmod(path, _S_IWRITE);
 #endif
     return remove(path);
 }
@@ -526,9 +526,7 @@ static FILE* FIO_openSrcFile(const char* srcFileName)
     }
 
     if (!UTIL_isRegularFile(srcFileName)
-#ifndef _MSC_VER
+     && !UTIL_isFIFO(srcFileName)
-        && !UTIL_isFIFO(srcFileName)
-#endif /* _MSC_VER */
     ) {
         DISPLAYLEVEL(1, "zstd: %s is not a regular file -- ignored \n",
                         srcFileName);
@@ -609,8 +607,11 @@ FIO_openDstFile(FIO_prefs_t* const prefs,
     {   FILE* const f = fopen( dstFileName, "wb" );
         if (f == NULL) {
             DISPLAYLEVEL(1, "zstd: %s: %s\n", dstFileName, strerror(errno));
-        } else if(srcFileName != NULL && strcmp (srcFileName, stdinmark)) {
+        } else if (srcFileName != NULL
-            chmod(dstFileName, 00600);
+               && strcmp (srcFileName, stdinmark)
+               && strcmp(dstFileName, nulmark) ) {
+            /* reduce rights on newly created dst file while compression is ongoing */
+            UTIL_chmod(dstFileName, 00600);
         }
         return f;
     }
@@ -1393,7 +1394,7 @@ static int FIO_compressFilename_dstFile(FIO_prefs_t* const prefs,
     assert(ress.srcFile != NULL);
     if (ress.dstFile == NULL) {
         closeDstFile = 1;
-        DISPLAYLEVEL(6, "FIO_compressFilename_dstFile: opening dst: %s", dstFileName);
+        DISPLAYLEVEL(6, "FIO_compressFilename_dstFile: opening dst: %s \n", dstFileName);
         ress.dstFile = FIO_openDstFile(prefs, srcFileName, dstFileName);
         if (ress.dstFile==NULL) return 1;  /* could not open dstFileName */
         /* Must only be added after FIO_openDstFile() succeeds.
@@ -1415,6 +1416,7 @@ static int FIO_compressFilename_dstFile(FIO_prefs_t* const prefs,
 
         clearHandler();
 
+        DISPLAYLEVEL(6, "FIO_compressFilename_dstFile: closing dst: %s \n", dstFileName);
         if (fclose(dstFile)) { /* error closing dstFile */
             DISPLAYLEVEL(1, "zstd: %s: %s \n", dstFileName, strerror(errno));
             result=1;
@@ -1427,7 +1429,10 @@ static int FIO_compressFilename_dstFile(FIO_prefs_t* const prefs,
         } else if ( strcmp(dstFileName, stdoutmark)
                  && strcmp(dstFileName, nulmark)
                  && transfer_permissions) {
+            DISPLAYLEVEL(6, "FIO_compressFilename_dstFile: transfering permissions into dst: %s \n", dstFileName);
             UTIL_setFileStat(dstFileName, &statbuf);
+        } else {
+            DISPLAYLEVEL(6, "FIO_compressFilename_dstFile: do not transfer permissions into dst: %s \n", dstFileName);
         }
     }
 
@@ -1462,6 +1467,7 @@ FIO_compressFilename_srcFile(FIO_prefs_t* const prefs,
                              int compressionLevel)
 {
     int result;
+    DISPLAYLEVEL(6, "FIO_compressFilename_srcFile: %s \n", srcFileName);
 
     /* ensure src is not a directory */
     if (UTIL_isDirectory(srcFileName)) {

programs/util.c

@@ -24,6 +24,21 @@ extern "C" {
 #include <direct.h>     /* needed for _mkdir in windows */
 #endif
 
+#if defined(_MSC_VER)
+    #define chmod _chmod
+#endif
+
+
+/*-*************************************
+*  Constants
+***************************************/
+#define LIST_SIZE_INCREASE   (8*1024)
+
+
+/*-*************************************
+*  Functions
+***************************************/
+
 int UTIL_fileExist(const char* filename)
 {
     stat_t statbuf;
@@ -54,6 +69,13 @@ int UTIL_getFileStat(const char* infilename, stat_t *statbuf)
     return 1;
 }
 
+/* like chmod, but avoid changing permission of /dev/null */
+int UTIL_chmod(char const* filename, mode_t permissions)
+{
+    if (!strcmp(filename, "/dev/null")) return 0;   /* pretend success, but don't change anything */
+    return chmod(filename, permissions);
+}
+
 int UTIL_setFileStat(const char *filename, stat_t *statbuf)
 {
     int res = 0;
@@ -82,21 +104,20 @@ int UTIL_setFileStat(const char *filename, stat_t *statbuf)
     res += chown(filename, statbuf->st_uid, statbuf->st_gid);  /* Copy ownership */
 #endif
 
-    res += chmod(filename, statbuf->st_mode & 07777);  /* Copy file permissions */
+    res += UTIL_chmod(filename, statbuf->st_mode & 07777);  /* Copy file permissions */
 
     errno = 0;
     return -res; /* number of errors is returned */
 }
 
-U32 UTIL_isDirectory(const char* infilename)
+int UTIL_isDirectory(const char* infilename)
 {
-    int r;
     stat_t statbuf;
 #if defined(_MSC_VER)
-    r = _stat64(infilename, &statbuf);
+    int const r = _stat64(infilename, &statbuf);
     if (!r && (statbuf.st_mode & _S_IFDIR)) return 1;
 #else
-    r = stat(infilename, &statbuf);
+    int const r = stat(infilename, &statbuf);
     if (!r && S_ISDIR(statbuf.st_mode)) return 1;
 #endif
     return 0;
@@ -126,28 +147,25 @@ int UTIL_isSameFile(const char* fName1, const char* fName2)
 #endif
 }
 
-#ifndef _MSC_VER
+/* UTIL_isFIFO : distinguish named pipes */
-/* Using this to distinguish named pipes */
+int UTIL_isFIFO(const char* infilename)
-U32 UTIL_isFIFO(const char* infilename)
 {
 /* macro guards, as defined in : https://linux.die.net/man/2/lstat */
 #if PLATFORM_POSIX_VERSION >= 200112L
     stat_t statbuf;
-    int r = UTIL_getFileStat(infilename, &statbuf);
+    int const r = UTIL_getFileStat(infilename, &statbuf);
     if (!r && S_ISFIFO(statbuf.st_mode)) return 1;
 #endif
     (void)infilename;
     return 0;
 }
-#endif
 
-U32 UTIL_isLink(const char* infilename)
+int UTIL_isLink(const char* infilename)
 {
 /* macro guards, as defined in : https://linux.die.net/man/2/lstat */
 #if PLATFORM_POSIX_VERSION >= 200112L
-    int r;
     stat_t statbuf;
-    r = lstat(infilename, &statbuf);
+    int const r = lstat(infilename, &statbuf);
     if (!r && S_ISLNK(statbuf.st_mode)) return 1;
 #endif
     (void)infilename;

programs/util.h

@@ -30,12 +30,12 @@ extern "C" {
 #  include <io.h>         /* _chmod */
 #else
 #  include <unistd.h>     /* chown, stat */
-#if PLATFORM_POSIX_VERSION < 200809L
+#  if PLATFORM_POSIX_VERSION < 200809L
-#  include <utime.h>      /* utime */
+#    include <utime.h>      /* utime */
-#else
+#  else
-#  include <fcntl.h>      /* AT_FDCWD */
+#    include <fcntl.h>      /* AT_FDCWD */
-#  include <sys/stat.h>   /* utimensat */
+#    include <sys/stat.h>   /* utimensat */
-#endif
+#  endif
 #endif
 #include <time.h>         /* clock_t, clock, CLOCKS_PER_SEC, nanosleep */
 #include "mem.h"          /* U32, U64 */
@@ -85,12 +85,6 @@ extern "C" {
 #endif
 
 
-/*-*************************************
-*  Constants
-***************************************/
-#define LIST_SIZE_INCREASE   (8*1024)
-
-
 /*-****************************************
 *  Compiler specifics
 ******************************************/
@@ -120,8 +114,8 @@ extern int g_utilDisplayLevel;
 *  File functions
 ******************************************/
 #if defined(_MSC_VER)
-    #define chmod _chmod
     typedef struct __stat64 stat_t;
+    typedef int mode_t;
 #else
     typedef struct stat stat_t;
 #endif
@@ -129,30 +123,29 @@ extern int g_utilDisplayLevel;
 
 int UTIL_fileExist(const char* filename);
 int UTIL_isRegularFile(const char* infilename);
-int UTIL_setFileStat(const char* filename, stat_t* statbuf);
+int UTIL_isDirectory(const char* infilename);
-U32 UTIL_isDirectory(const char* infilename);
-int UTIL_getFileStat(const char* infilename, stat_t* statbuf);
 int UTIL_isSameFile(const char* file1, const char* file2);
-int UTIL_compareStr(const void *p1, const void *p2);
 int UTIL_isCompressedFile(const char* infilename, const char *extensionList[]);
-const char* UTIL_getFileExtension(const char* infilename);
+int UTIL_isLink(const char* infilename);
+int UTIL_isFIFO(const char* infilename);
 
-#ifndef _MSC_VER
-U32 UTIL_isFIFO(const char* infilename);
-#endif
-U32 UTIL_isLink(const char* infilename);
 #define UTIL_FILESIZE_UNKNOWN  ((U64)(-1))
 U64 UTIL_getFileSize(const char* infilename);
-
 U64 UTIL_getTotalFileSize(const char* const * const fileNamesTable, unsigned nbFiles);
+int UTIL_getFileStat(const char* infilename, stat_t* statbuf);
+int UTIL_setFileStat(const char* filename, stat_t* statbuf);
+int UTIL_chmod(char const* filename, mode_t permissions);   /*< like chmod, but avoid changing permission of /dev/null */
+int UTIL_compareStr(const void *p1, const void *p2);
+const char* UTIL_getFileExtension(const char* infilename);
+
 
 /*
  * A modified version of realloc().
  * If UTIL_realloc() fails the original block is freed.
 */
-UTIL_STATIC void* UTIL_realloc(void *ptr, size_t size)
+UTIL_STATIC void* UTIL_realloc(void* ptr, size_t size)
 {
-    void *newptr = realloc(ptr, size);
+    void* const newptr = realloc(ptr, size);
     if (newptr) return newptr;
     free(ptr);
     return NULL;

programs/zstdcli.c

@@ -1005,16 +1005,13 @@ int main(int argCount, const char* argv[])
     if (!followLinks) {
         unsigned u;
         for (u=0, fileNamesNb=0; u<filenameIdx; u++) {
-            if (UTIL_isLink(filenameTable[u])
+            if ( UTIL_isLink(filenameTable[u])
-#ifndef _MSC_VER
+             && !UTIL_isFIFO(filenameTable[u])
-                && !UTIL_isFIFO(filenameTable[u])
-#endif /* _MSC_VER */
             ) {
                 DISPLAYLEVEL(2, "Warning : %s is a symbolic link, ignoring\n", filenameTable[u]);
             } else {
                 filenameTable[fileNamesNb++] = filenameTable[u];
-            }
+        }   }
-        }
         if (fileNamesNb == 0 && filenameIdx > 0)
             CLEAN_RETURN(1);
         filenameIdx = fileNamesNb;
@@ -1027,8 +1024,7 @@ int main(int argCount, const char* argv[])
             free((void*)filenameTable);
             filenameTable = extendedFileList;
             filenameIdx = fileNamesNb;
-        }
+    }   }
-    }
 #else
     (void)followLinks;
 #endif
@@ -1074,18 +1070,15 @@ int main(int argCount, const char* argv[])
                     DISPLAYLEVEL(3, "Benchmarking %s \n", filenameTable[i]);
                     for(c = cLevel; c <= cLevelLast; c++) {
                         BMK_benchFilesAdvanced(&filenameTable[i], 1, dictFileName, c, &compressionParams, g_displayLevel, &benchParams);
-                    }
+                }   }
-                }
             } else {
                 for(; cLevel <= cLevelLast; cLevel++) {
                     BMK_benchFilesAdvanced(filenameTable, filenameIdx, dictFileName, cLevel, &compressionParams, g_displayLevel, &benchParams);
-                }
+            }   }
-            }
         } else {
             for(; cLevel <= cLevelLast; cLevel++) {
                 BMK_syntheticTest(cLevel, compressibility, &compressionParams, g_displayLevel, &benchParams);
-            }
+        }   }
-        }
 
 #else
         (void)bench_nbSeconds; (void)blockSize; (void)setRealTimePrio; (void)separateFiles; (void)compressibility;

tests/playTests.sh

@@ -220,13 +220,12 @@ $ZSTD tmp -c --compress-literals    -19      | $ZSTD -t
 $ZSTD -b --fast=1 -i0e1 tmp --compress-literals
 $ZSTD -b --fast=1 -i0e1 tmp --no-compress-literals
 
-println "test: --exclude-compressed flag"
+println "\n===>  --exclude-compressed flag"
 rm -rf precompressedFilterTestDir
 mkdir -p precompressedFilterTestDir
 ./datagen $size > precompressedFilterTestDir/input.5
 ./datagen $size > precompressedFilterTestDir/input.6
 $ZSTD --exclude-compressed --long --rm -r precompressedFilterTestDir
-sleep 5
 ./datagen $size > precompressedFilterTestDir/input.7
 ./datagen $size > precompressedFilterTestDir/input.8
 $ZSTD --exclude-compressed --long --rm -r precompressedFilterTestDir
@@ -251,7 +250,7 @@ test -f precompressedFilterTestDir/input.5.zst.zst
 test -f precompressedFilterTestDir/input.6.zst.zst
 println "Test completed"
 
-println "test : file removal"
+println "\n===>  file removal"
 $ZSTD -f --rm tmp
 test ! -f tmp  # tmp should no longer be present
 $ZSTD -f -d --rm tmp.zst
@@ -278,13 +277,16 @@ $ZSTD -f tmp && die "attempt to compress a non existing file"
 test -f tmp.zst  # destination file should still be present
 rm -rf tmp*  # may also erase tmp* directory from previous failed run
 
-println "\n===> decompression only tests "
+println "\n===>  decompression only tests "
-dd bs=1 count=1048576 if=/dev/zero of=tmp
+# the following test verifies that the decoder is compatible with RLE as first block
+# older versions of zstd cli are not able to decode such corner case.
+# As a consequence, the zstd cli do not generate them, to maintain compatibility with older versions.
+dd bs=1048576 count=1 if=/dev/zero of=tmp
 $ZSTD -d -o tmp1 "$TESTDIR/golden-decompression/rle-first-block.zst"
 $DIFF -s tmp1 tmp
 rm tmp*
 
-println "test : compress multiple files"
+println "\m===>  compress multiple files"
 println hello > tmp1
 println world > tmp2
 $ZSTD tmp1 tmp2 -o "$INTOVOID" -f
@@ -306,7 +308,21 @@ if [ "$?" -eq 139 ]; then
 fi
 rm tmp*
 
-println "test : compress multiple files into an output directory, --output-dir-flat"
+if [ -n "$DEVNULLRIGHTS" ]
+then
+    # these tests requires sudo rights, which is uncommon.
+    # they are only triggered if DEVNULLRIGHTS macro is defined.
+    println "\n===> checking /dev/null permissions are unaltered "
+    ./datagen > tmp
+    sudo $ZSTD tmp -o $INTOVOID   # sudo rights could modify /dev/null permissions
+    sudo $ZSTD tmp -c > $INTOVOID
+    $ZSTD tmp -f -o tmp.zst
+    sudo $ZSTD -d tmp.zst -c > $INTOVOID
+    sudo $ZSTD -d tmp.zst -o $INTOVOID
+    ls -las $INTOVOID | grep "rw-rw-rw-"
+fi
+
+println "\n===>  compress multiple files into an output directory, --output-dir-flat"
 println henlo > tmp1
 mkdir tmpInputTestDir
 mkdir tmpInputTestDir/we
@@ -352,7 +368,6 @@ $ZSTD -dcf tmp1
 
 
 println "\n===>  frame concatenation "
-
 println "hello " > hello.tmp
 println "world!" > world.tmp
 cat hello.tmp world.tmp > helloworld.tmp