From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6344 invoked by alias); 18 Oct 2013 16:38:33 -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 6334 invoked by uid 89); 18 Oct 2013 16:38:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 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 16:38:32 +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 r9IGcUtt013018 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 18 Oct 2013 12:38:30 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r9IGcSt0030358; Fri, 18 Oct 2013 12:38:29 -0400 Message-ID: <52616404.2020604@redhat.com> Date: Fri, 18 Oct 2013 16:38:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: gdb-patches@sourceware.org, dcb314@hotmail.com Subject: Re: [PATCH v2][PR gdb/16013] Fix off-by-one errors in *scanf format strings References: <20131014105252.GA5262@blade.nx> <525BD49B.4080700@redhat.com> <20131018143903.GA14055@blade.nx> In-Reply-To: <20131018143903.GA14055@blade.nx> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-10/txt/msg00576.txt.bz2 On 10/18/2013 03:39 PM, Gary Benson wrote: > 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 I think it'd be beneficial for readability. I'd prefer making it 18 then. > (does it make a difference if things are declared to be a power of two in > size?) I don't think so. In any case, this code is not really a hot path or that size sensitive. > 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. That's fine. > > 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? > Yes, this looks excellent! Thanks a lot for doing all this. > 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", > + Spurious newline? > local_address, &local_port, -- Pedro Alves