From: Michael Snyder <msnyder@vmware.com>
To: Mark Kettenis <mark.kettenis@xs4all.nl>
Cc: "jan.kratochvil@redhat.com" <jan.kratochvil@redhat.com>,
"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: Re: [RFA] i386-tdep.c, check target_read_memory for error.
Date: Tue, 08 Mar 2011 18:47:00 -0000 [thread overview]
Message-ID: <4D767783.4090703@vmware.com> (raw)
In-Reply-To: <201103072226.p27MQ8Zp004631@glazunov.sibelius.xs4all.nl>
[-- Attachment #1: Type: text/plain, Size: 6717 bytes --]
Mark Kettenis wrote:
>> Date: Sun, 06 Mar 2011 11:00:12 -0800
>> From: Michael Snyder <msnyder@vmware.com>
>>
>> Mark Kettenis wrote:
>>>> Date: Sun, 6 Mar 2011 15:15:16 +0100
>>>> From: Jan Kratochvil <jan.kratochvil@redhat.com>
>>>>
>>>> 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 <msnyder@vmware.com>
>>
>> * 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
[-- Attachment #2: target_read4.txt --]
[-- Type: text/plain, Size: 5339 bytes --]
2011-03-08 Michael Snyder <msnyder@vmware.com>
* 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];
next prev parent reply other threads:[~2011-03-08 18:38 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-03-04 21:38 Michael Snyder
2011-03-06 14:56 ` Jan Kratochvil
2011-03-06 17:00 ` Mark Kettenis
2011-03-06 22:34 ` Michael Snyder
2011-03-08 2:35 ` Mark Kettenis
2011-03-08 18:47 ` Michael Snyder [this message]
2011-03-08 18:59 ` Mark Kettenis
2011-03-08 19:27 ` Pedro Alves
2011-03-08 19:41 ` Mark Kettenis
2011-03-08 19:50 ` Pedro Alves
2011-03-09 1:32 ` Michael Snyder
2011-03-06 18:48 ` Michael Snyder
2011-03-06 19:00 ` Jan Kratochvil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4D767783.4090703@vmware.com \
--to=msnyder@vmware.com \
--cc=gdb-patches@sourceware.org \
--cc=jan.kratochvil@redhat.com \
--cc=mark.kettenis@xs4all.nl \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox