Command line parsing issue(s?) causing undefined behaviour
Opened this issue · 3 comments
VorpalBlade commented
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 commented
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 commented
$ echo a > -v $ cachestats -v
Shoot yourself in the foot, then complain about the gunsmiths.
pavlinux commented
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;
}