From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3019 invoked by alias); 18 Oct 2013 14:39:11 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 3005 invoked by uid 89); 18 Oct 2013 14:39:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.7 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 18 Oct 2013 14:39:09 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r9IEd5S2031405 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 18 Oct 2013 10:39:06 -0400 Received: from blade.nx (ovpn-116-100.ams2.redhat.com [10.36.116.100]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r9IEd4m9005026; Fri, 18 Oct 2013 10:39:04 -0400 Received: by blade.nx (Postfix, from userid 1000) id 7242E264B83; Fri, 18 Oct 2013 15:39:03 +0100 (BST) Date: Fri, 18 Oct 2013 14:39:00 -0000 From: Gary Benson To: Pedro Alves Cc: gdb-patches@sourceware.org, dcb314@hotmail.com Subject: [PATCH v2][PR gdb/16013] Fix off-by-one errors in *scanf format strings Message-ID: <20131018143903.GA14055@blade.nx> Mail-Followup-To: Pedro Alves , gdb-patches@sourceware.org, dcb314@hotmail.com References: <20131014105252.GA5262@blade.nx> <525BD49B.4080700@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <525BD49B.4080700@redhat.com> X-IsSubscribed: yes X-SW-Source: 2013-10/txt/msg00564.txt.bz2 Pedro Alves wrote: > These could be fixed by either reducing the length specified > in the format string, or, by increasing the buffers. Either > such change would be obvious from a coding perspective. But > the part that requires a rationale, is, that one that justifies > the taken approach. That will be governed what the actual lengths > of these fields on the kernel side. E.g.: > > /* sizeof (cmd) should be greater or equal to TASK_COMM_LEN (in > include/linux/sched.h in the Linux kernel sources) plus two > (for the brackets). */ > char cmd[32]; > PID_T stat_pid; > int items_read = fscanf (fp, "%lld %32s", &stat_pid, cmd); > > Did you check the value of TASK_COMM_LEN ? (I haven't). > > Same for the other fields. Ok, I think I have now checked *everything* :) In the first hunk, the maximum size is 16 (including the terminator). Add two for the brackets makes 18, so 32 is big enough. I don't know if there would be any benefit to reducing the buffer here to 18 (does it make a difference if things are declared to be a power of two in size?) For the second hunk I caused all the unused variables to be skipped, which took care of the buffer "extra" being too small. I also reduced the size specifiers of local_address and remote_address from 33 to 32. This is the correct size (for IPv6) but was not causing an overflow as NI_MAXHOST is 1025 on my machine. I added a compile-time check for this, but don't know if this is overkill. In the third hunk, the dependencies field could be arbitrarily long, so I've rewritten it using strtok. While doing this I noticed that size (an unsigned int) is parsed as "%d" but printed as "%u" so I changed the parsing to "%u". I left untouched the funky indentation of the lines following the buffer_xml_printf but could change them to something else if required. Is this ok? Thanks, Gary -- http://gbenson.net/ 2013-10-18 Gary Benson PR 16013 * common/linux-osdata.c (command_from_pid): Modified fscanf format string to avoid leaving cmd unterminated. (print_sockets): Do not parse tlen, inode, sl, timeout, txq, rxq, trun, retn or extra. (Avoids leaving extra unterminated.) Check that local_address and remote_address will not overflow. (linux_xfer_osdata_modules): Parse lines using strtok to avoid leaving dependencies unterminated. Parse size as "%u" to match definition. diff --git a/gdb/common/linux-osdata.c b/gdb/common/linux-osdata.c index 9723839..29c3d77 100644 --- a/gdb/common/linux-osdata.c +++ b/gdb/common/linux-osdata.c @@ -137,7 +137,7 @@ command_from_pid (char *command, int maxlen, PID_T pid) (for the brackets). */ char cmd[32]; PID_T stat_pid; - int items_read = fscanf (fp, "%lld %32s", &stat_pid, cmd); + int items_read = fscanf (fp, "%lld %31s", &stat_pid, cmd); if (items_read == 2 && pid == stat_pid) { @@ -871,29 +871,23 @@ print_sockets (unsigned short family, int tcp, struct buffer *buffer) if (fgets (buf, sizeof (buf), fp)) { uid_t uid; - unsigned long tlen, inode; - int sl, timeout; unsigned int local_port, remote_port, state; - unsigned int txq, rxq, trun, retn; char local_address[NI_MAXHOST], remote_address[NI_MAXHOST]; - char extra[512]; int result; +#if NI_MAXHOST <= 32 +#error "local_address and remote_address buffers too small" +#endif + result = sscanf (buf, - "%d: %33[0-9A-F]:%X %33[0-9A-F]:%X %X %X:%X %X:%lX %X %d %d %lu %512s\n", - &sl, + "%*d: %32[0-9A-F]:%X %32[0-9A-F]:%X %X %*X:%*X %*X:%*X %*X %d %*d %*u %*s\n", + local_address, &local_port, remote_address, &remote_port, &state, - &txq, &rxq, - &trun, &tlen, - &retn, - &uid, - &timeout, - &inode, - extra); + &uid); - if (result == 15) + if (result == 6) { union socket_addr locaddr, remaddr; size_t addr_size; @@ -1464,19 +1458,42 @@ linux_xfer_osdata_modules (gdb_byte *readbuf, { if (fgets (buf, sizeof (buf), fp)) { - char name[64], dependencies[256], status[16]; + char *name, *dependencies, *status, *tmp; unsigned int size; unsigned long long address; int uses; - int items_read; - - items_read = sscanf (buf, - "%64s %d %d %256s %16s 0x%llx", - name, &size, &uses, - dependencies, status, &address); - if (items_read == 6) - buffer_xml_printf ( + name = strtok (buf, " "); + if (name == NULL) + continue; + + tmp = strtok (NULL, " "); + if (tmp == NULL) + continue; + if (sscanf (tmp, "%u", &size) != 1) + continue; + + tmp = strtok (NULL, " "); + if (tmp == NULL) + continue; + if (sscanf (tmp, "%d", &uses) != 1) + continue; + + dependencies = strtok (NULL, " "); + if (dependencies == NULL) + continue; + + status = strtok (NULL, " "); + if (status == NULL) + continue; + + tmp = strtok (NULL, "\n"); + if (tmp == NULL) + continue; + if (sscanf (tmp, "%llx", &address) != 1) + continue; + + buffer_xml_printf ( &buffer, "" "%s"