From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25635 invoked by alias); 6 Mar 2011 19:00:24 -0000 Received: (qmail 25624 invoked by uid 22791); 6 Mar 2011 19:00:22 -0000 X-SWARE-Spam-Status: No, hits=-5.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp-outbound-2.vmware.com (HELO smtp-outbound-2.vmware.com) (65.115.85.73) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 06 Mar 2011 19:00:13 +0000 Received: from mailhost4.vmware.com (mailhost4.vmware.com [10.16.67.124]) by smtp-outbound-2.vmware.com (Postfix) with ESMTP id 6992927002; Sun, 6 Mar 2011 11:00:12 -0800 (PST) Received: from msnyder-server.eng.vmware.com (promd-2s-dhcp138.eng.vmware.com [10.20.124.138]) by mailhost4.vmware.com (Postfix) with ESMTP id 61BA1C9F8F; Sun, 6 Mar 2011 11:00:12 -0800 (PST) Message-ID: <4D73D9BC.6020705@vmware.com> Date: Sun, 06 Mar 2011 22:34:00 -0000 From: Michael Snyder User-Agent: Thunderbird 2.0.0.24 (X11/20101201) MIME-Version: 1.0 To: Mark Kettenis CC: "jan.kratochvil@redhat.com" , "gdb-patches@sourceware.org" Subject: Re: [RFA] i386-tdep.c, check target_read_memory for error. References: <4D715BB0.8030506@vmware.com> <20110306141515.GA1895@host1.jankratochvil.net> <201103061455.p26Etppr028003@glazunov.sibelius.xs4all.nl> In-Reply-To: <201103061455.p26Etppr028003@glazunov.sibelius.xs4all.nl> Content-Type: multipart/mixed; boundary="------------010208080808000004020805" X-IsSubscribed: yes 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 X-SW-Source: 2011-03/txt/msg00406.txt.bz2 This is a multi-part message in MIME format. --------------010208080808000004020805 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 1450 Mark Kettenis wrote: >> Date: Sun, 6 Mar 2011 15:15:16 +0100 >> From: Jan Kratochvil >> >> On Fri, 04 Mar 2011 22:37:52 +0100, Michael Snyder wrote: >>> Call error if target_read_memory fails. >> [...] >>> - target_read_memory (pc, &op, 1); >>> + if (target_read_memory (pc, &op, 1)) >>> + error (_("Couldn't read memory at pc (%s)"), >>> + paddress (gdbarch, pc)); >> There is the function `read_memory' for such purpose. > > But read_memory() will throw an exception if reading fails. That is > not necessarily what we want here. In fact, most of these reads > should silently fail. They are part of the prologue analysis code, > which to some of extent is based on heuristics. And one of the > heristics here is that if we fail to read an instruction at a certain > address, we're no longer looking at a function prologue. Higher level > code will try an alternative strategy or issue an error message. > Spamming the user with more error messages isn't going to be terribly > helpful. > > But Michael is right that there is an issue here. The code is relying > on uninitialized stack variables not matching the specific opcodes we > check against. I think most of the: > > target_read_memory(pc, &op, 1); > > statements, should be replaced with > > if (target_read_memory(pc, &op, 1)) > return pc; > > Cheers, > > Mark Thanks. So changed. Will you give it an eyeball? Michael --------------010208080808000004020805 Content-Type: text/plain; name="target_read2.txt" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="target_read2.txt" Content-length: 8062 2011-03-05 Michael Snyder * i386-tdep.c (i386_follow_jump): Check target_read_memory for error. (i386_analyze_struct_return): Add gdbarch parameter. Check target_read_memory for error. (i386_skip_probe): Ditto. (i386_match_insn): Ditto. (i386_skip_noop): Ditto. (i386_analyze_register_saves): Ditto. (i386_analyze_frame_setup): Pass gdbarch to i386_match_insn. Check target_read_memory for error. (i386_analyze_prologue): Pass gdbarch to sub-functions. (i386_skip_prologue): Check target_read_memory for error. (i386_skip_main_prologue): Ditto. Index: i386-tdep.c =================================================================== RCS file: /cvs/src/src/gdb/i386-tdep.c,v retrieving revision 1.324 diff -u -p -r1.324 i386-tdep.c --- i386-tdep.c 8 Feb 2011 14:01:47 -0000 1.324 +++ i386-tdep.c 6 Mar 2011 18:58:31 -0000 @@ -850,7 +850,9 @@ i386_follow_jump (struct gdbarch *gdbarc long delta = 0; int data16 = 0; - target_read_memory (pc, &op, 1); + if (target_read_memory (pc, &op, 1)) + return pc; + if (op == 0x66) { data16 = 1; @@ -895,7 +897,8 @@ i386_follow_jump (struct gdbarch *gdbarc whichever is smaller. Otherwise, return PC. */ static CORE_ADDR -i386_analyze_struct_return (CORE_ADDR pc, CORE_ADDR current_pc, +i386_analyze_struct_return (struct gdbarch *gdbarch, CORE_ADDR pc, + CORE_ADDR current_pc, struct i386_frame_cache *cache) { /* Functions that return a structure or union start with: @@ -916,12 +919,15 @@ i386_analyze_struct_return (CORE_ADDR pc if (current_pc <= pc) return pc; - target_read_memory (pc, &op, 1); + if (target_read_memory (pc, &op, 1)) + return pc; if (op != 0x58) /* popl %eax */ return pc; - target_read_memory (pc + 1, buf, 4); + if (target_read_memory (pc + 1, buf, 4)) + return pc + 1; + if (memcmp (buf, proto1, 3) != 0 && memcmp (buf, proto2, 4) != 0) return pc; @@ -944,7 +950,7 @@ i386_analyze_struct_return (CORE_ADDR pc } static CORE_ADDR -i386_skip_probe (CORE_ADDR pc) +i386_skip_probe (struct gdbarch *gdbarch, CORE_ADDR pc) { /* A function may start with @@ -960,7 +966,8 @@ i386_skip_probe (CORE_ADDR pc) gdb_byte buf[8]; gdb_byte op; - target_read_memory (pc, &op, 1); + if (target_read_memory (pc, &op, 1)) + return pc; if (op == 0x68 || op == 0x6a) { @@ -1116,12 +1123,14 @@ struct i386_insn NULL. */ static struct i386_insn * -i386_match_insn (CORE_ADDR pc, struct i386_insn *skip_insns) +i386_match_insn (struct gdbarch *gdbarch, CORE_ADDR pc, + struct i386_insn *skip_insns) { struct i386_insn *insn; gdb_byte op; - target_read_memory (pc, &op, 1); + if (target_read_memory (pc, &op, 1)) + return pc; for (insn = skip_insns; insn->len > 0; insn++) { @@ -1134,7 +1143,9 @@ i386_match_insn (CORE_ADDR pc, struct i3 gdb_assert (insn->len > 1); gdb_assert (insn->len <= I386_MAX_MATCHED_INSN_LEN); - target_read_memory (pc + 1, buf, insn->len - 1); + if (target_read_memory (pc + 1, buf, insn->len - 1)) + return pc; + for (i = 1; i < insn->len; i++) { if ((buf[i - 1] & insn->mask[i]) != insn->insn[i]) @@ -1207,12 +1218,13 @@ struct i386_insn i386_frame_setup_skip_i /* Check whether PC points to a no-op instruction. */ static CORE_ADDR -i386_skip_noop (CORE_ADDR pc) +i386_skip_noop (struct gdbarch *gdbarch, CORE_ADDR pc) { gdb_byte op; int check = 1; - target_read_memory (pc, &op, 1); + if (target_read_memory (pc, &op, 1)) + return pc; while (check) { @@ -1221,7 +1233,8 @@ i386_skip_noop (CORE_ADDR pc) if (op == 0x90) { pc += 1; - target_read_memory (pc, &op, 1); + if (target_read_memory (pc, &op, 1)) + return pc; check = 1; } /* Ignore no-op instruction `mov %edi, %edi'. @@ -1237,11 +1250,15 @@ i386_skip_noop (CORE_ADDR pc) else if (op == 0x8b) { - target_read_memory (pc + 1, &op, 1); + if (target_read_memory (pc + 1, &op, 1)) + return pc + 1; + if (op == 0xff) { pc += 2; - target_read_memory (pc, &op, 1); + if (target_read_memory (pc, &op, 1)) + return pc; + check = 1; } } @@ -1267,7 +1284,8 @@ i386_analyze_frame_setup (struct gdbarch if (limit <= pc) return limit; - target_read_memory (pc, &op, 1); + if (target_read_memory (pc, &op, 1)) + error (_("Couldn't read memory at %s"), paddress (gdbarch, pc)); if (op == 0x55) /* pushl %ebp */ { @@ -1291,7 +1309,8 @@ i386_analyze_frame_setup (struct gdbarch `movl %esp, %ebp' that actually sets up the frame. */ while (pc + skip < limit) { - insn = i386_match_insn (pc + skip, i386_frame_setup_skip_insns); + insn = i386_match_insn (gdbarch, pc + skip, + i386_frame_setup_skip_insns); if (insn == NULL) break; @@ -1302,7 +1321,8 @@ i386_analyze_frame_setup (struct gdbarch if (limit <= pc + skip) return limit; - target_read_memory (pc + skip, &op, 1); + if (target_read_memory (pc + skip, &op, 1)) + return pc + skip; /* Check for `movl %esp, %ebp' -- can be written in two ways. */ switch (op) @@ -1338,7 +1358,8 @@ i386_analyze_frame_setup (struct gdbarch NOTE: You can't subtract a 16-bit immediate from a 32-bit reg, so we don't have to worry about a data16 prefix. */ - target_read_memory (pc, &op, 1); + if (target_read_memory (pc, &op, 1)) + return pc; if (op == 0x83) { /* `subl' with 8-bit immediate. */ @@ -1383,7 +1404,8 @@ i386_analyze_frame_setup (struct gdbarch smaller. Otherwise, return PC. */ static CORE_ADDR -i386_analyze_register_saves (CORE_ADDR pc, CORE_ADDR current_pc, +i386_analyze_register_saves (struct gdbarch *gdbarch, CORE_ADDR pc, + CORE_ADDR current_pc, struct i386_frame_cache *cache) { CORE_ADDR offset = 0; @@ -1394,7 +1416,8 @@ i386_analyze_register_saves (CORE_ADDR p offset -= cache->locals; for (i = 0; i < 8 && pc < current_pc; i++) { - target_read_memory (pc, &op, 1); + if (target_read_memory (pc, &op, 1)) + return pc; if (op < 0x50 || op > 0x57) break; @@ -1439,13 +1462,13 @@ i386_analyze_prologue (struct gdbarch *g CORE_ADDR pc, CORE_ADDR current_pc, struct i386_frame_cache *cache) { - pc = i386_skip_noop (pc); + pc = i386_skip_noop (gdbarch, pc); pc = i386_follow_jump (gdbarch, pc); - pc = i386_analyze_struct_return (pc, current_pc, cache); - pc = i386_skip_probe (pc); + pc = i386_analyze_struct_return (gdbarch, pc, current_pc, cache); + pc = i386_skip_probe (gdbarch, pc); pc = i386_analyze_stack_align (pc, current_pc, cache); pc = i386_analyze_frame_setup (gdbarch, pc, current_pc, cache); - return i386_analyze_register_saves (pc, current_pc, cache); + return i386_analyze_register_saves (gdbarch, pc, current_pc, cache); } /* Return PC of first real instruction. */ @@ -1487,7 +1510,9 @@ i386_skip_prologue (struct gdbarch *gdba for (i = 0; i < 6; i++) { - target_read_memory (pc + i, &op, 1); + if (target_read_memory (pc + i, &op, 1)) + return pc + i; + if (pic_pat[i] != op) break; } @@ -1495,7 +1520,9 @@ i386_skip_prologue (struct gdbarch *gdba { int delta = 6; - target_read_memory (pc + delta, &op, 1); + if (target_read_memory (pc + delta, &op, 1)) + return pc + delta; + if (op == 0x89) /* movl %ebx, x(%ebp) */ { @@ -1508,7 +1535,9 @@ i386_skip_prologue (struct gdbarch *gdba else /* Unexpected instruction. */ delta = 0; - target_read_memory (pc + delta, &op, 1); + if (target_read_memory (pc + delta, &op, 1)) + return pc + delta; + } /* addl y,%ebx */ @@ -1538,7 +1567,8 @@ i386_skip_main_prologue (struct gdbarch enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); gdb_byte op; - target_read_memory (pc, &op, 1); + if (target_read_memory (pc, &op, 1)) + return pc; if (op == 0xe8) { gdb_byte buf[4]; --------------010208080808000004020805--