From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20154 invoked by alias); 25 May 2005 06:09:02 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 19737 invoked by uid 22791); 25 May 2005 06:08:54 -0000 Received: from ausmtp02.au.ibm.com (HELO ausmtp02.au.ibm.com) (202.81.18.187) by sourceware.org (qpsmtpd/0.30-dev) with ESMTP; Wed, 25 May 2005 06:08:54 +0000 Received: from sd0208e0.au.ibm.com (d23rh904.au.ibm.com [202.81.18.202]) by ausmtp02.au.ibm.com (8.12.10/8.12.10) with ESMTP id j4P6442F111188; Wed, 25 May 2005 16:04:07 +1000 Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.250.242]) by sd0208e0.au.ibm.com (8.12.10/NCO/VER6.6) with ESMTP id j4P6B1a8102248; Wed, 25 May 2005 16:11:05 +1000 Received: from d23av01.au.ibm.com (loopback [127.0.0.1]) by d23av01.au.ibm.com (8.12.11/8.13.3) with ESMTP id j4P686PW029383; Wed, 25 May 2005 16:08:06 +1000 Received: from plinuxt18.cn.ibm.com (plinuxt18.cn.ibm.com [9.181.140.28]) by d23av01.au.ibm.com (8.12.11/8.12.11) with ESMTP id j4P683nV029171; Wed, 25 May 2005 16:08:04 +1000 Date: Wed, 25 May 2005 13:00:00 -0000 From: Wu Zhou To: gdb@sources.redhat.com, gdb-patches@sources.redhat.com cc: mark.kettenis@xs4all.nl, eliz@gnu.org Subject: about the usage of sprintf in gdb, specifically in gdb/remote.c Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2005-05/txt/msg00541.txt.bz2 Mark, Eli and others who might be interested in this topic, To continue the discussion on the usage of sprintf in gdb, I am now trying to get out some pattern of it in the source code, specifically in gdb/remote.c (It happens that I am reading its source recently). I summarize my findings below. Would you please take a look and give comments. Any errors, please feel free to correct me. Thanks a lot. 1. Won't introduce overflow. And one can easily see this from the context. To name an example in function remote_thread_alive: int tid = PIDGET (ptid); char buf[16]; if (tid < 0) sprintf (buf, "T-%08x", -tid); else sprintf (buf, "T%08x", tid); In this context, there is no possibility that buf get overflowed. IMHO, there is no need to switch to safer alternatives with this usage pattern. 2. Won't introduce overflow, but it is somewhat harder to see this from the context. To name an example in function remote_threads_extra_info: char *bufp = alloca (rs->remote_packet_size); ... if (use_threadextra_query) { sprintf (bufp, "qThreadExtraInfo,%x", PIDGET (tp->ptid)); ... To determine whether there is any possibility to overflow bufp in this context, we need to determine: first the size of bufp, then the size of PIDGET (tp->ptid). To determine the sizeof of bufp, we need to determine the range of rs->remote_packet_size. Althought it turns out that this is much larger than what could be put into it, and this won't introduce any overflow. But I still consider this kind of usage to be hard-to-determine one. My idea on this is to define a macro: #define REMOTE_PACKET_SIZE (rs->remote_packet_size) then use xsnprintf to replace the original sprintf call: xsnprintf (bufp, REMOTE_PACKTE_SIZE, "qThreadExtraInfo,%x", PIDGET (tp->ptid)); There are about dozen such usages of sprintf in gdb/remote.c. 3. Might introduce overflow. Investigating the source code of remote.c, I find there are two usages of sprintf, which might introduce overflow: 3.1 sprintf on display_buf, which is also in remote_threads_extra_info display_buf is a char array with the size of 100: static char display_buf[100]; /* arbitrary... */ But it is coded to hold three members of struct gdb_ext_thread_info (shortname, display and more_display): if (remote_get_threadinfo (&id, set, &threadinfo)) if (threadinfo.active) { if (*threadinfo.shortname) n += sprintf(&display_buf[0], " Name: %s,", threadinfo.shortname); if (*threadinfo.display) n += sprintf(&display_buf[n], " State: %s,", threadinfo.display); if (*threadinfo.more_display) n += sprintf(&display_buf[n], " Priority: %s", threadinfo.more_display); The size of these three members will be 544 if added together (Refer to the following declaration): struct gdb_ext_thread_info { threadref threadid; /* External form of thread reference. */ int active; /* Has state interesting to GDB? regs, stack. */ char display[256]; /* Brief state display, name, blocked/suspended. */ char shortname[32]; /* To be used to name threads. */ char more_display[256]; /* Long info, statistics, queue depth, whatever. */ }; In my opinion, this is easy to incur an overflow. But I don't have chance to design a scenario to verify this. What is your thought on this? 3.2 sprintf on msg, which is in function remote_check_symbols Please have a look at the following code section: msg = alloca (rs->remote_packet_size); reply = alloca (rs->remote_packet_size); /* Invite target to request symbol lookups. */ putpkt ("qSymbol::"); getpkt (reply, (rs->remote_packet_size), 0); packet_ok (reply, &remote_protocol_qSymbol); while (strncmp (reply, "qSymbol:", 8) == 0) { tmp = &reply[8]; end = hex2bin (tmp, msg, strlen (tmp) / 2); msg[end] = '\0'; sym = lookup_minimal_symbol (msg, NULL, NULL); if (sym == NULL) sprintf (msg, "qSymbol::%s", &reply[8]); else sprintf (msg, "qSymbol:%s:%s", paddr_nz (SYMBOL_VALUE_ADDRESS (sym)), &reply[8]); through alloca, msg and reply get a memory region with the size of rs->remote_packet_size. If the size of reply goes beyond (rs->remote_packet_size - 1), the first sprintf call will be overflowed. To overflow the second sprintf call, the size of reply could be smaller. I am not sure how long reply will get to (In my experiecne, some c++ symbols could get to very long). I am also not sure how long rs->remote_packet_size will get to. So it depends on the difference between these two values whether this could incur overflow. Maybe this won't incur any overflow at last. But I think the usage of sprintf here is highly suspectable. Any different thought on this? Thanks for your attention on this long mail. Cheers - Wu Zhou