diff mbox series

[2/2] gdb/riscv: Fix buffer overflow on riscv_insn::fetch_instruction

Message ID 89612fe01d902007bf84a7dfb0df5f85d5c166e4.1664873933.git.research_trasio@irq.a4lg.com
State Superseded
Headers
Series RISC-V: Fix buffer overflow after long instruction support |

Commit Message

Tsukasa OI Oct. 4, 2022, 8:59 a.m. UTC 
  Because riscv_insn_length started to support instructions up to 176-bit,
we need to increase packet buffer size to 176-bit in size.

Note that this change will make the result of riscv_insn::fetch_instruction
partial when the instruction is longer than 64-bits.  To really support
instructions longer than 64-bit, we need something more.
---
 gdb/riscv-tdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andreas Schwab Oct. 4, 2022, 9:04 a.m. UTC | #1 
On Okt 04 2022, Tsukasa OI via Gdb-patches wrote:

> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 47d8f9e601b..99307bd2de1 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -1770,7 +1770,7 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
>  			       CORE_ADDR addr, int *len)
>  {
>    enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
> -  gdb_byte buf[8];
> +  gdb_byte buf[22];

Can the magic number be derived from something else so that is adapts
automatically?
Tsukasa OI Oct. 4, 2022, 9:14 a.m. UTC | #2 
On 2022/10/04 18:04, Andreas Schwab wrote:
> On Okt 04 2022, Tsukasa OI via Gdb-patches wrote:
> 
>> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
>> index 47d8f9e601b..99307bd2de1 100644
>> --- a/gdb/riscv-tdep.c
>> +++ b/gdb/riscv-tdep.c
>> @@ -1770,7 +1770,7 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
>>  			       CORE_ADDR addr, int *len)
>>  {
>>    enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
>> -  gdb_byte buf[8];
>> +  gdb_byte buf[22];
> 
> Can the magic number be derived from something else so that is adapts
> automatically?
> 

Well...
That's technically possible but it will make something like...
    8 + 2 + 2 * ((1 << 3) - 1 - 1)
and I don't think that would improve something (it heavily depends on
the encoding magic [no relevant constants are defined so far] so it's
just "from magic to magic").

Defining RISCV_MAX_INSN_LEN might be a slightly better solution (still,
content of RISCV_MAX_INSN_LEN will be a magic number).

Thanks,
Tsukasa
Andreas Schwab Oct. 4, 2022, 9:25 a.m. UTC | #3 
On Okt 04 2022, Tsukasa OI wrote:

> That's technically possible but it will make something like...
>     8 + 2 + 2 * ((1 << 3) - 1 - 1)

That's still a magic number that doesn't adapt.

> Defining RISCV_MAX_INSN_LEN might be a slightly better solution (still,
> content of RISCV_MAX_INSN_LEN will be a magic number).

If that number is then used as the base for the other dependencies, it's
a win.
Tsukasa OI Oct. 4, 2022, 9:28 a.m. UTC | #4 
On 2022/10/04 18:25, Andreas Schwab wrote:
> On Okt 04 2022, Tsukasa OI wrote:
> 
>> That's technically possible but it will make something like...
>>     8 + 2 + 2 * ((1 << 3) - 1 - 1)
> 
> That's still a magic number that doesn't adapt.

Exactly.  I could not find any constants to depend to.  If we define
"currently" magic number (22) as RISCV_MAX_INSN_LEN, at least we can
share that constant between Binutils and GDB.

> 
>> Defining RISCV_MAX_INSN_LEN might be a slightly better solution (still,
>> content of RISCV_MAX_INSN_LEN will be a magic number).
> 
> If that number is then used as the base for the other dependencies, it's
> a win.
>
diff mbox series

Patch

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 47d8f9e601b..99307bd2de1 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1770,7 +1770,7 @@  riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
 			       CORE_ADDR addr, int *len)
 {
   enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
-  gdb_byte buf[8];
+  gdb_byte buf[22];
   int instlen, status;
 
   /* All insns are at least 16 bits.  */