From owner-svn-src-head@freebsd.org Tue May 24 23:36:45 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 25E76B49990; Tue, 24 May 2016 23:36:45 +0000 (UTC) (envelope-from truckman@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id E781D1EA7; Tue, 24 May 2016 23:36:44 +0000 (UTC) (envelope-from truckman@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id u4ONaijP081179; Tue, 24 May 2016 23:36:44 GMT (envelope-from truckman@FreeBSD.org) Received: (from truckman@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id u4ONait8081178; Tue, 24 May 2016 23:36:44 GMT (envelope-from truckman@FreeBSD.org) Message-Id: <201605242336.u4ONait8081178@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: truckman set sender to truckman@FreeBSD.org using -f From: Don Lewis Date: Tue, 24 May 2016 23:36:44 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r300632 - head/usr.sbin/acpi/acpidump X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 May 2016 23:36:45 -0000 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