svn commit: r300632 - head/usr.sbin/acpi/acpidump

[Don Lewis]

Author: truckman
Date: Tue May 24 23:36:43 2016
New Revision: 300632
URL: https://svnweb.freebsd.org/changeset/base/300632

Log:
  Fix acpidump CID 1011278 (Buffer not null terminated) and other issues
  
  Coverity reports that a buffer used for temporary file generation
  might not be NUL terminated by strncpy().  This is probably not
  true because the input gets passed through realpath(), but if the
  path name is sufficiently long the name could be truncated and cause
  other problems.  The code for generating the temp file names is
  also overly complex.  Instead of a bunch of calls to strncpy() and
  and strncat(), simplify the code by using snprintf() and add checks
  for unexpected truncation.
  
  The output file created by iasl -d is predictable.  Fix this by
  using  mkdtemp() to create a directory to hold the iasl input and
  output files.
  
  Check the return values of more syscalls.
  
  Reported by:	Coverity
  CID:		1011278
  Reviewed by:	jkim
  MFC after:	1 week
  Differential Revision:	https://reviews.freebsd.org/D6360

Modified:
  head/usr.sbin/acpi/acpidump/acpi.c

Modified: head/usr.sbin/acpi/acpidump/acpi.c
==============================================================================
--- head/usr.sbin/acpi/acpidump/acpi.c	Tue May 24 23:19:03 2016	(r300631)
+++ head/usr.sbin/acpi/acpidump/acpi.c	Tue May 24 23:36:43 2016	(r300632)
@@ -1466,27 +1466,34 @@ dsdt_save_file(char *outfile, ACPI_TABLE
 void
 aml_disassemble(ACPI_TABLE_HEADER *rsdt, ACPI_TABLE_HEADER *dsdp)
 {
-	char buf[PATH_MAX], tmpstr[PATH_MAX];
+	char buf[PATH_MAX], tmpstr[PATH_MAX], wrkdir[PATH_MAX];
+	const char *iname = "/acpdump.din";
+	const char *oname = "/acpdump.dsl";
 	const char *tmpdir;
-	char *tmpext;
 	FILE *fp;
 	size_t len;
-	int fd;
+	int fd, status;
+	pid_t pid;
 
 	tmpdir = getenv("TMPDIR");
 	if (tmpdir == NULL)
 		tmpdir = _PATH_TMP;
-	strncpy(tmpstr, tmpdir, sizeof(tmpstr));
-	if (realpath(tmpstr, buf) == NULL) {
+	if (realpath(tmpdir, buf) == NULL) {
 		perror("realpath tmp dir");
 		return;
 	}
-	strncpy(tmpstr, buf, sizeof(tmpstr));
-	strncat(tmpstr, "/acpidump.", sizeof(tmpstr) - strlen(buf));
-	len = strlen(tmpstr);
-	tmpext = tmpstr + len;
-	strncpy(tmpext, "XXXXXX", sizeof(tmpstr) - len);
-	fd = mkstemp(tmpstr);
+	len = sizeof(wrkdir) - strlen(iname);
+	if ((size_t)snprintf(wrkdir, len, "%s/acpidump.XXXXXX", buf) > len-1 ) {
+		fprintf(stderr, "$TMPDIR too long\n");
+		return;
+	}
+	if  (mkdtemp(wrkdir) == NULL) {
+		perror("mkdtemp tmp working dir");
+		return;
+	}
+	assert((size_t)snprintf(tmpstr, sizeof(tmpstr), "%s%s", wrkdir, iname)
+		<= sizeof(tmpstr) - 1);
+	fd = open(tmpstr, O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR);
 	if (fd < 0) {
 		perror("iasl tmp file");
 		return;
@@ -1495,28 +1502,46 @@ aml_disassemble(ACPI_TABLE_HEADER *rsdt,
 	close(fd);
 
 	/* Run iasl -d on the temp file */
-	if (fork() == 0) {
+	if ((pid = fork()) == 0) {
 		close(STDOUT_FILENO);
 		if (vflag == 0)
 			close(STDERR_FILENO);
 		execl("/usr/sbin/iasl", "iasl", "-d", tmpstr, NULL);
 		err(1, "exec");
 	}
-
-	wait(NULL);
-	unlink(tmpstr);
+	if (pid > 0)
+		wait(&status);
+	if (unlink(tmpstr) < 0) {
+		perror("unlink");
+		goto out;
+	}
+	if (pid < 0) {
+		perror("fork");
+		goto out;
+	}
+	if (status != 0) {
+		fprintf(stderr, "iast exit status = %d\n", status);
+	}
 
 	/* Dump iasl's output to stdout */
-	strncpy(tmpext, "dsl", sizeof(tmpstr) - len);
+	assert((size_t)snprintf(tmpstr, sizeof(tmpstr), "%s%s", wrkdir, oname)
+		<= sizeof(tmpstr) -1);
 	fp = fopen(tmpstr, "r");
-	unlink(tmpstr);
+	if (unlink(tmpstr) < 0) {
+		perror("unlink");
+		goto out;
+	}
 	if (fp == NULL) {
 		perror("iasl tmp file (read)");
-		return;
+		goto out;
 	}
 	while ((len = fread(buf, 1, sizeof(buf), fp)) > 0)
 		fwrite(buf, 1, len, stdout);
 	fclose(fp);
+
+    out:
+	if (rmdir(wrkdir) < 0)
+		perror("rmdir");
 }
 
 void