From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 663 invoked by alias); 8 Mar 2011 18:38:11 -0000 Received: (qmail 412 invoked by uid 22791); 8 Mar 2011 18:38:06 -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-1.vmware.com (HELO smtp-outbound-1.vmware.com) (65.115.85.69) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 08 Mar 2011 18:37:59 +0000 Received: from mailhost3.vmware.com (mailhost3.vmware.com [10.16.27.45]) by smtp-outbound-1.vmware.com (Postfix) with ESMTP id EF99213446; Tue, 8 Mar 2011 10:37:55 -0800 (PST) Received: from msnyder-server.eng.vmware.com (promd-2s-dhcp138.eng.vmware.com [10.20.124.138]) by mailhost3.vmware.com (Postfix) with ESMTP id DCC14CD9B2; Tue, 8 Mar 2011 10:37:55 -0800 (PST) Message-ID: <4D767783.4090703@vmware.com> Date: Tue, 08 Mar 2011 18:47: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> <4D73D9BC.6020705@vmware.com> <201103072226.p27MQ8Zp004631@glazunov.sibelius.xs4all.nl> In-Reply-To: <201103072226.p27MQ8Zp004631@glazunov.sibelius.xs4all.nl> Content-Type: multipart/mixed; boundary="------------090807040303000008010100" 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/msg00554.txt.bz2 This is a multi-part message in MIME format. --------------090807040303000008010100 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 6717 Mark Kettenis wrote: >> Date: Sun, 06 Mar 2011 11:00:12 -0800 >> From: Michael Snyder >> >> 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 >> >> 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. > > Hmm, several functions now have a gdbarch parameter that is unused. > >> 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 >> @@ -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; > > I think you want to return NULL here. > >> 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; > > And here too. > >> @@ -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; > > This should be a "return pc" > >> @@ -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)); > > Overlooked this one? > >> @@ -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; >> + > > This is checking for a very specific sequence. If we don't match the > complete sequence, we should probably return the address of the start > of the sequence, not the address of the partial match. So this should > be "return pc". > >> @@ -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; > > Same here. > >> @@ -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; > > and here. > >> + > > and please don't introduce spurious whitespace here. Thanks for your perseverance. New patch attached. There's still one more "return pc + skip" in there, which you didn't mention. Should I take it out? Michael --------------090807040303000008010100 Content-Type: text/plain; name="target_read4.txt" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="target_read4.txt" Content-length: 5339 2011-03-08 Michael Snyder * i386-tdep.c (i386_follow_jump): Check return value of target_read_memory. (i386_analyze_struct_return): Ditto. (i386_skip_probe): Ditto. (i386_match_insn): Ditto. (i386_skip_noop): Ditto. (i386_analyze_frame_setup): Ditto. (i386_analyze_register_saves): Ditto. (i386_skip_prologue): Ditto. (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 8 Mar 2011 18:32:01 -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; @@ -916,12 +918,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; + if (memcmp (buf, proto1, 3) != 0 && memcmp (buf, proto2, 4) != 0) return pc; @@ -960,7 +965,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) { @@ -1121,7 +1127,8 @@ i386_match_insn (CORE_ADDR pc, struct i3 struct i386_insn *insn; gdb_byte op; - target_read_memory (pc, &op, 1); + if (target_read_memory (pc, &op, 1)) + return NULL; for (insn = skip_insns; insn->len > 0; insn++) { @@ -1134,7 +1141,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 NULL; + for (i = 1; i < insn->len; i++) { if ((buf[i - 1] & insn->mask[i]) != insn->insn[i]) @@ -1212,7 +1221,8 @@ i386_skip_noop (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 +1231,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 +1248,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; + if (op == 0xff) { pc += 2; - target_read_memory (pc, &op, 1); + if (target_read_memory (pc, &op, 1)) + return pc; + check = 1; } } @@ -1267,7 +1282,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)) + return pc; if (op == 0x55) /* pushl %ebp */ { @@ -1302,7 +1318,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 +1355,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. */ @@ -1394,7 +1412,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; @@ -1487,7 +1506,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; + if (pic_pat[i] != op) break; } @@ -1495,7 +1516,8 @@ 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; if (op == 0x89) /* movl %ebx, x(%ebp) */ { @@ -1508,7 +1530,8 @@ 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; } /* addl y,%ebx */ @@ -1538,7 +1561,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]; --------------090807040303000008010100--