[PATCH] mtd-utils: tests: jittertest: reject overlong file names

[Aviv Daum <aviv.daum@gmail.com>]

plotJittervsFill copies the -f argument into a 250-byte buffer with
strncpy(..., sizeof(LogFile)). A 250-byte file name leaves the buffer
unterminated, and the subsequent fopen() reads past the end of LogFile.

JitterTest uses the same fixed-size file name pattern for -r, while -c
still silently truncates overlong names and -f already rejects them.

Validate jittertest file name arguments before copying them so these
options all reject overlong input instead of truncating it or reading
past the end of fixed-size buffers.

Add a shell regression test that exercises the accepted and rejected
boundary lengths for plotJittervsFill and JitterTest during make check.

Signed-off-by: Aviv Daum <aviv.daum@gmail.com>
---
 Makefile.am                            |   3 +
 configure.ac                           |   1 +
 tests/jittertest/JitterTest.c          |  52 +++++++----
 tests/jittertest/Makemodule.am         |   6 +-
 tests/jittertest/filename_bounds.sh.in | 115 +++++++++++++++++++++++++
 tests/jittertest/plotJittervsFill.c    |  17 +++-
 6 files changed, 175 insertions(+), 19 deletions(-)
 create mode 100755 tests/jittertest/filename_bounds.sh.in

diff --git a/Makefile.am b/Makefile.am
index c756127..ba54acc 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -42,6 +42,9 @@ test_SCRIPTS =
 test_DATA =
 
 TESTS =
+TEST_EXTENSIONS = .sh
+SH_LOG_COMPILER = $(SHELL)
+AM_TESTS_ENVIRONMENT = TESTBINDIR='$(abs_top_builddir)'; export TESTBINDIR;
 EXTRA_DIST = COPYING CHANGELOG.md README.txt
 
 include lib/Makemodule.am
diff --git a/configure.ac b/configure.ac
index 2a79ba8..f17d703 100644
--- a/configure.ac
+++ b/configure.ac
@@ -304,6 +304,7 @@ AC_CONFIG_FILES([tests/fs-tests/fs_help_all.sh
 	tests/fs-tests/fs_run_all.sh
 	tests/fs-tests/stress/fs_stress00.sh
 	tests/fs-tests/stress/fs_stress01.sh
+	tests/jittertest/filename_bounds.sh
 	tests/ubi-tests/runubitests.sh
 	tests/ubi-tests/ubi-stress-test.sh
 	tests/ubifs_tools-tests/lib/common.sh
diff --git a/tests/jittertest/JitterTest.c b/tests/jittertest/JitterTest.c
index 2bee0b0..a3e3764 100644
--- a/tests/jittertest/JitterTest.c
+++ b/tests/jittertest/JitterTest.c
@@ -205,6 +205,7 @@ static int RunAsRTTask = FALSE; /* default action unless priority is
 
 /********************* Local Function Prototypes **********************/
 void HandleCmdLineArgs(int argc, char *argv[]);
+static void SaveFileName(char *pDest, size_t destSize, const char *pFileName);
 void SetFileName(char * pFileName);
 void SetInterruptPeriod(char * pASCIIInterruptPeriodMilliSec);
 void SetSchedulerPriority(char * pASCIISchedulerPriority);
@@ -830,9 +831,14 @@ void HandleCmdLineArgs(
                      (strcmp(argv[argNum],"-r")       == STRINGS_EQUAL)) {
                 /* Set the file to read*/
                 ++argNum;
-
-                strncpy(ReadFile, argv[argNum], sizeof(ReadFile));
-                DoRead = TRUE;
+                if (argNum < argc) {
+                    SaveFileName(ReadFile, sizeof(ReadFile), argv[argNum]);
+                    DoRead = TRUE;
+                }
+                else {
+                    printf("*** Read file name not specified. ***\n");
+                    exit(0);
+                }
             }
 
             else if ((strcmp(argv[argNum],"--write_bytes") ==
@@ -858,9 +864,13 @@ void HandleCmdLineArgs(
                      (strcmp(argv[argNum],"-c")       == STRINGS_EQUAL)) {
 	      /* Set the file to log console log on. */
 	      ++argNum;
-
-	      strncpy(LogFile, argv[argNum], sizeof(LogFile) - 1);
-	      LogFile[sizeof(LogFile) - 1] = '\0';
+	      if (argNum < argc) {
+	          SaveFileName(LogFile, sizeof(LogFile), argv[argNum]);
+	      }
+	      else {
+	          printf("*** Console log file name not specified. ***\n");
+	          exit(0);
+	      }
             }
 
             else if ((strcmp(argv[argNum],"--grab_kprofile") ==
@@ -913,27 +923,37 @@ void HandleCmdLineArgs(
 
 
 /***********************************************************************
- *                                SetFileName
- *  This function sets the output file name.
+ *                              SaveFileName
+ *  This function validates and saves a file name.
  *  output: N/A
  ***********************************************************************/
-void SetFileName(
-    char * pFileName)               /* ptr to desired output file name  */
+static void SaveFileName(
+    char *pDest,                    /* ptr to destination buffer        */
+    size_t destSize,                /* destination buffer size          */
+    const char *pFileName)          /* ptr to desired file name         */
 {
     size_t fileNameLen;             /* file name length (bytes)         */
 
-    /* Check file name length. */
     fileNameLen = strlen(pFileName);
-    if (fileNameLen > (size_t) MAX_FILE_NAME_LEN) {
+    if (fileNameLen > destSize - 1) {
         printf("File name %s exceeds maximum length %d.\n",
-               pFileName, MAX_FILE_NAME_LEN);
+               pFileName, (int)(destSize - 1));
         exit(0);
     }
 
-    /* File name length is OK so save the file name. */
-    strcpy(OutFileName, pFileName);
+    strcpy(pDest, pFileName);
+}
 
-    return;
+
+/***********************************************************************
+ *                                SetFileName
+ *  This function sets the output file name.
+ *  output: N/A
+ ***********************************************************************/
+void SetFileName(
+    char * pFileName)               /* ptr to desired output file name  */
+{
+    SaveFileName(OutFileName, sizeof(OutFileName), pFileName);
 }
 
 
diff --git a/tests/jittertest/Makemodule.am b/tests/jittertest/Makemodule.am
index d280192..2cb85c1 100644
--- a/tests/jittertest/Makemodule.am
+++ b/tests/jittertest/Makemodule.am
@@ -6,6 +6,8 @@ plotJittervsFill_CPPFLAGS = $(AM_CPPFLAGS)
 
 test_PROGRAMS += JitterTest plotJittervsFill
 
-test_SCRIPTS += tests/jittertest/filljffs2.sh
+test_SCRIPTS += tests/jittertest/filljffs2.sh tests/jittertest/filename_bounds.sh
+TESTS += tests/jittertest/filename_bounds.sh
 
-EXTRA_DIST += tests/jittertest/README tests/jittertest/filljffs2.sh
+EXTRA_DIST += tests/jittertest/README tests/jittertest/filljffs2.sh \
+	tests/jittertest/filename_bounds.sh.in
diff --git a/tests/jittertest/filename_bounds.sh.in b/tests/jittertest/filename_bounds.sh.in
new file mode 100755
index 0000000..0dfba74
--- /dev/null
+++ b/tests/jittertest/filename_bounds.sh.in
@@ -0,0 +1,115 @@
+#!/bin/sh
+
+TESTBINDIR=${TESTBINDIR-@TESTBINDIR@}
+
+tmpdir=
+jt_pid=
+watchdog_pid=
+
+fatal()
+{
+	echo "Error: $1" 1>&2
+	exit 1
+}
+
+cleanup()
+{
+	if [ -n "$watchdog_pid" ]; then
+		kill "$watchdog_pid" >/dev/null 2>&1 || :
+		wait "$watchdog_pid" 2>/dev/null || :
+	fi
+
+	if [ -n "$jt_pid" ]; then
+		kill "$jt_pid" >/dev/null 2>&1 || :
+		wait "$jt_pid" 2>/dev/null || :
+	fi
+
+	if [ -n "$tmpdir" ]; then
+		rm -rf "$tmpdir"
+	fi
+}
+
+trap 'status=$?; trap - EXIT; cleanup; exit $status' EXIT
+trap 'exit 1' HUP INT QUIT TERM
+
+make_name()
+{
+	char="$1"
+	length="$2"
+	name=
+	i=0
+
+	while [ "$i" -lt "$length" ]; do
+		name="${name}${char}"
+		i=$((i + 1))
+	done
+
+	printf '%s' "$name"
+}
+
+tmpdir=$(mktemp -d "${TMPDIR:-/tmp}/mtd-utils-jittertest.XXXXXX") ||
+	fatal "mktemp failed"
+cd "$tmpdir" || fatal "cannot change to temp dir"
+
+plot_valid=$(make_name a 249)
+: > "$plot_valid" || fatal "cannot create valid plot input"
+"$TESTBINDIR/plotJittervsFill" -f "$plot_valid" >plot-valid.out 2>plot-valid.err ||
+	fatal "plotJittervsFill rejected a 249-byte file name"
+if grep -q "exceeds maximum length" plot-valid.out plot-valid.err; then
+	fatal "plotJittervsFill reported a max-length error for a 249-byte file name"
+fi
+
+plot_invalid=$(make_name b 250)
+"$TESTBINDIR/plotJittervsFill" -f "$plot_invalid" >plot-invalid.out 2>plot-invalid.err || :
+if ! grep -q "exceeds maximum length" plot-invalid.out plot-invalid.err; then
+	fatal "plotJittervsFill did not reject a 250-byte file name"
+fi
+if grep -q "Unable to open input log file" plot-invalid.out plot-invalid.err; then
+	fatal "plotJittervsFill reached fopen() for an overlong file name"
+fi
+
+jt_read_valid=$(make_name r 32)
+"$TESTBINDIR/JitterTest" -c /dev/null -f out.dat -r "$jt_read_valid" >jt-valid.out 2>jt-valid.err &
+jt_pid=$!
+(
+	sleep 10
+	kill -TERM "$jt_pid" >/dev/null 2>&1 || :
+	sleep 1
+	kill -KILL "$jt_pid" >/dev/null 2>&1 || :
+) &
+watchdog_pid=$!
+sleep 1
+kill -INT "$jt_pid" >/dev/null 2>&1 || fatal "cannot stop JitterTest"
+wait "$jt_pid" || fatal "JitterTest failed with a 32-byte read file name"
+jt_pid=
+kill "$watchdog_pid" >/dev/null 2>&1 || :
+wait "$watchdog_pid" 2>/dev/null || :
+watchdog_pid=
+if ! grep -q "Press Ctrl+C to exit the program." jt-valid.out; then
+	fatal "JitterTest did not start normally with a 32-byte read file name"
+fi
+if ! grep -q "JitterTest exiting." jt-valid.out; then
+	fatal "JitterTest did not exit cleanly after SIGINT"
+fi
+
+jt_read_invalid=$(make_name s 33)
+"$TESTBINDIR/JitterTest" -c /dev/null -f out.dat -r "$jt_read_invalid" \
+	>jt-read-invalid.out 2>jt-read-invalid.err || :
+if ! grep -q "exceeds maximum length" jt-read-invalid.out jt-read-invalid.err; then
+	fatal "JitterTest did not reject a 33-byte read file name"
+fi
+if grep -q "Press Ctrl+C to exit the program." jt-read-invalid.out jt-read-invalid.err; then
+	fatal "JitterTest started despite an overlong read file name"
+fi
+
+jt_console_invalid=$(make_name c 33)
+"$TESTBINDIR/JitterTest" -c "$jt_console_invalid" -f out.dat \
+	>jt-console-invalid.out 2>jt-console-invalid.err || :
+if ! grep -q "exceeds maximum length" jt-console-invalid.out jt-console-invalid.err; then
+	fatal "JitterTest did not reject a 33-byte console file name"
+fi
+if grep -q "Press Ctrl+C to exit the program." jt-console-invalid.out jt-console-invalid.err; then
+	fatal "JitterTest started despite an overlong console file name"
+fi
+
+echo "SUCCESS"
diff --git a/tests/jittertest/plotJittervsFill.c b/tests/jittertest/plotJittervsFill.c
index 03929a9..8929f9a 100644
--- a/tests/jittertest/plotJittervsFill.c
+++ b/tests/jittertest/plotJittervsFill.c
@@ -75,6 +75,21 @@ static int Debug = 0; /* Debug level. Each "-d" on the cmd line increases the le
 
 #define MIN_JITTER_THRESHOLD 1 /* ms minimum jitter threshold */
 
+static void SetLogFileName(
+    const char *pFileName)          /* ptr to desired input file name   */
+{
+    size_t fileNameLen;             /* file name length (bytes)         */
+
+    fileNameLen = strlen(pFileName);
+    if (fileNameLen > sizeof(LogFile) - 1) {
+        printf("File name %s exceeds maximum length %d.\n",
+               pFileName, (int)(sizeof(LogFile) - 1));
+        exit(0);
+    }
+
+    strcpy(LogFile, pFileName);
+}
+
 static void PrintHelpInfo(void)
 {
     printf("Usage: plotJittervsFill [options] -f [--file] <input log file name> -t [--jitter_threshold] <jitter threshold in ms>\n");
@@ -122,7 +137,7 @@ static void HandleCmdLineArgs(
                 /* Set the name of the output file. */
                 ++argNum;
                 if (argNum < argc) {
-                    strncpy(LogFile, argv[argNum], sizeof(LogFile));
+                    SetLogFileName(argv[argNum]);
                 }
                 else {
                     printf("*** Input file name not specified. ***\n");
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/