Command line parsing issue(s?) causing undefined behaviour

[VorpalBlade]

Version 1.1 from package in Ubuntu 20.04 (x86-64). Probably not a major or exploitable issue as the program isn't suid etc, but still should probably be fixed.

$ cachestats -v
open: Bad address

$ valgrind cachestats -v
==196391== Memcheck, a memory error detector
==196391== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==196391== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
==196391== Command: cachestats -v
==196391== 
==196391== Syscall param openat(filename) points to unaddressable byte(s)
==196391==    at 0x49A6EAB: open (open64.c:48)
==196391==    by 0x1091C1: open (fcntl2.h:53)
==196391==    by 0x1091C1: main (cachestats.c:49)
==196391==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==196391== 
open: Bad address
==196391== 
==196391== HEAP SUMMARY:
==196391==     in use at exit: 0 bytes in 0 blocks
==196391==   total heap usage: 2 allocs, 2 frees, 1,496 bytes allocated
==196391== 
==196391== All heap blocks were freed -- no leaks are possible
==196391== 
==196391== For lists of detected and suppressed errors, rerun with: -s
==196391== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

It appears the issue is passing a single flag and no file. Also happens with -q.

Also while not leading to any undefined behaviour, the parsing seems a bit odd with cachedel as well:

$ cachedel 
usage: cachedel [-n <n>] <file> -- call fadvise(DONTNEED) <n> times on file
$ cachedel -n
open: No such file or directory
$ cachedel -n 2
usage: cachedel [-n <n>] <file> -- call fadvise(DONTNEED) <n> times on file

That middle line doesn't seem to follow the pattern.

Finally, it appears -- works with cachedel but not cachestats. Incredibly unlikely use case, but for robustness this should probably work:

$ echo a > -v
$ cachestats -v
open: Bad address
$ cachestats -- -v
open: No such file or directory

Note, cachestats works with files starting with - as long as they are not exactly -v or -q. And cachedel seems to implement the -- option to treat the remaining command line as non-flags.

[VorpalBlade]

Looking at the code of cachedel I have no idea why I thought it handled -- correctly. It did not give me any error, which made me trust it worked correctly. I now believe it simply ended up doing nothing somehow.

[pavlinux]

$ echo a > -v
$ cachestats -v

Shoot yourself in the foot, then complain about the gunsmiths.

[pavlinux]

index 7e9d9f6..27b6e2b 100644
--- a/cachestats.c
+++ b/cachestats.c
@@ -1,9 +1,11 @@
+#define _GNU_SOURCE /* O_DIRECT */
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <sys/mman.h>
 #include <unistd.h>
 #include <stdlib.h>
+#include <getopt.h>
 #include <string.h>
 #include <error.h>
 #include <stdio.h>
@@ -14,6 +16,14 @@ int exiterr(const char *s)
     exit(-1);
 }

+static const struct option longOpts[] = {
+    { "verbose", required_argument, NULL, 'v' },
+    { "quiet",   required_argument, NULL, 'q' },
+    { "help",    no_argument,       NULL, 'h' },
+    { "?",       no_argument,       NULL, '?' },
+    { NULL,      no_argument,       NULL, 0 }
+};
+
 int main(int argc, char *argv[])
 {
     int i, j;
@@ -22,57 +32,85 @@ int main(int argc, char *argv[])

     int quiet = 0;
     int verbose = 0;
+    int opt = -1;
+    int long_index = -1;

     int fd;
     struct stat st;
     void *file = NULL;
+    char *name = NULL;
     unsigned char *pageinfo = NULL;

     PAGESIZE = getpagesize();

-    if(argc > 1) {
-        if(!strcmp("-v", argv[1]))
-            verbose = 1;
-        else if(!strcmp("-q", argv[1]))
-            quiet = 1;
+    if (argv[2]) {
+    while ((opt = getopt_long_only(argc, argv, "", longOpts, &long_index)) != -1) {
+   /* short arg will be filename */
+   switch (opt) {
+       case 'v':
+           verbose = 1;
+           name = optarg;
+           break;
+       case 'q':
+           quiet = 1;
+           name = optarg;
+           break;
+       case 'h':
+       case '?':
+       default:
+           fprintf(stderr, "usage: %s [--quiet | --verbose] <file> "
+               "-- print out cache statistics\n", argv[0]);
+           fprintf(stderr, "\t--verbose\tprint verbose cache map\n");
+           fprintf(stderr, "\t--quiet\texit code tells if file is fully cached\n");
+           exit(1);
+           break;
+   }
+      }
     } else {
-        fprintf(stderr, "usage: %s [-qv] <file> "
-            "-- print out cache statistics\n", argv[0]);
-        fprintf(stderr, "\t-v\tprint verbose cache map\n");
-        fprintf(stderr, "\t-q\texit code tells if file is fully cached\n");
-        exit(1);
+       name = argv[1];
     }

-    if(quiet || verbose)
-        argv++;
+    if (name == NULL)
+   exit(1);

-    fd = open(argv[1], O_RDONLY);
+    fd = open(name, O_RDONLY|O_DIRECT|O_SYNC|O_NDELAY);
     if(fd == -1)
         exiterr("open");

     if(fstat(fd, &st) == -1)
         exiterr("fstat");
     if(!S_ISREG(st.st_mode)) {
-        fprintf(stderr, "%s: S_ISREG: not a regular file", argv[1]);
+        fprintf(stderr, "%s: S_ISREG: not a regular file", optarg);
+        close(fd);
         return EXIT_FAILURE;
     }
     if(st.st_size == 0) {
-        printf("pages in cache: %d/%d (%.1f%%)  [filesize=%.1fK, "
-                "pagesize=%dK]\n", 0, 0, 0.0,
-                0.0, PAGESIZE / 1024);
+   if (verbose) {
+       printf("pages in cache: %d/%d (%.1f%%)  [filesize=%.1fK, "
+           "pagesize=%dK]\n", 0, 0, 0.0,
+           0.0, PAGESIZE / 1024);
+        }
+        close(fd);
         return EXIT_SUCCESS;
     }

     pages = (st.st_size + PAGESIZE - 1) / PAGESIZE;
     pageinfo = calloc(sizeof(*pageinfo), pages);
-    if(!pageinfo)
+    if(!pageinfo) {
+        close(fd);
         exiterr("calloc");
+    }

     file = mmap(NULL, st.st_size, PROT_NONE, MAP_SHARED, fd, 0);
-    if(file == MAP_FAILED)
+    if(file == MAP_FAILED) {
+        close(fd);
         exiterr("mmap");
-    if(mincore(file, st.st_size, pageinfo) == -1)
+    }
+    if(mincore(file, st.st_size, pageinfo) == -1) {
+        munmap(file, st.st_size);
+        close(fd);
         exiterr("mincore");
+    }

     i = j = 0;
     while(i < pages)
@@ -80,6 +118,8 @@ int main(int argc, char *argv[])
             j++;

     if(quiet) {
+        munmap(file, st.st_size);
+        close(fd);
         if(j == i)
             return EXIT_SUCCESS;
         return EXIT_FAILURE;
@@ -100,10 +140,11 @@ int main(int argc, char *argv[])
                 printf("%c|",
                     pageinfo[i * PAGES_PER_LINE + j] & 1 ? 'x' : ' ');
             }
-            printf("\n");
+        printf("\n");
         }
     }

     munmap(file, st.st_size);
+    close(fd);
     return EXIT_SUCCESS;
 }