[PATCH] Fix buffer overflow in ada-lang.c:move_bits

[Tom Tromey]

-fsanitize=address showed that ada-lang.c:move_bits can run off the
end of the source buffer. I believe this patch fixes the problem, by
arranging not to read from the source buffer once there are sufficient
bits in the accumulator.

gdb/ChangeLog
2018-10-23 Tom Tromey <***@tromey.com>

* ada-lang.c (move_bits): Don't run off the end of the source
buffer.
---
gdb/ChangeLog | 5 +++++
gdb/ada-lang.c | 18 ++++++++++++------
2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 1462271a71..7288d65df6 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2682,9 +2682,12 @@ move_bits (gdb_byte *target, int targ_offset, const gdb_byte *source,
{
int unused_right;

- accum = (accum << HOST_CHAR_BIT) + (unsigned char) *source;
- accum_bits += HOST_CHAR_BIT;
- source += 1;
+ if (n > accum_bits)
+ {
+ accum = (accum << HOST_CHAR_BIT) + (unsigned char) *source;
+ accum_bits += HOST_CHAR_BIT;
+ source += 1;
+ }
chunk_size = HOST_CHAR_BIT - targ_offset;
if (chunk_size > n)
chunk_size = n;
@@ -2707,9 +2710,12 @@ move_bits (gdb_byte *target, int targ_offset, const gdb_byte *source,

while (n > 0)
{
- accum = accum + ((unsigned char) *source << accum_bits);
- accum_bits += HOST_CHAR_BIT;
- source += 1;
+ if (n > accum_bits)
+ {
+ accum = accum + ((unsigned char) *source << accum_bits);
+ accum_bits += HOST_CHAR_BIT;
+ source += 1;
+ }
chunk_size = HOST_CHAR_BIT - targ_offset;
if (chunk_size > n)
chunk_size = n;

[Joel Brobecker]

Hi Tom,

Post by Tom Tromey
-fsanitize=address showed that ada-lang.c:move_bits can run off the
end of the source buffer. I believe this patch fixes the problem, by
arranging not to read from the source buffer once there are sufficient
bits in the accumulator.
gdb/ChangeLog
* ada-lang.c (move_bits): Don't run off the end of the source
buffer.

Thanks for the patch!

This is a part of the code that always forces me to think twice
(or ten times), each time I try to touch it. I should really start
adding comments to this code that detail what we are trying to do
as we do it.

I tested your change through our testsuite on the various baremetal
targets we have, and noticed that it causes regressions on ppc and arm
targets. It's hopefully something small, but just being back from
a holiday, I'm a bit tied up at work; I'll put that issue on my TODO
list to look at further.

Post by Tom Tromey
---
gdb/ChangeLog | 5 +++++
gdb/ada-lang.c | 18 ++++++++++++------
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 1462271a71..7288d65df6 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2682,9 +2682,12 @@ move_bits (gdb_byte *target, int targ_offset, const gdb_byte *source,
{
int unused_right;
- accum = (accum << HOST_CHAR_BIT) + (unsigned char) *source;
- accum_bits += HOST_CHAR_BIT;
- source += 1;
+ if (n > accum_bits)
+ {
+ accum = (accum << HOST_CHAR_BIT) + (unsigned char) *source;
+ accum_bits += HOST_CHAR_BIT;
+ source += 1;
+ }
chunk_size = HOST_CHAR_BIT - targ_offset;
if (chunk_size > n)
chunk_size = n;
@@ -2707,9 +2710,12 @@ move_bits (gdb_byte *target, int targ_offset, const gdb_byte *source,
while (n > 0)
{
- accum = accum + ((unsigned char) *source << accum_bits);
- accum_bits += HOST_CHAR_BIT;
- source += 1;
+ if (n > accum_bits)
+ {
+ accum = accum + ((unsigned char) *source << accum_bits);
+ accum_bits += HOST_CHAR_BIT;
+ source += 1;
+ }
chunk_size = HOST_CHAR_BIT - targ_offset;
if (chunk_size > n)
chunk_size = n;
--
2.17.1

[Tom Tromey]

Joel> I tested your change through our testsuite on the various baremetal
Joel> targets we have, and noticed that it causes regressions on ppc and arm
Joel> targets. It's hopefully something small, but just being back from
Joel> a holiday, I'm a bit tied up at work; I'll put that issue on my TODO
Joel> list to look at further.

Thanks. To reproduce the problem I saw, just rebuild with
-fsanitize=address and run the gdb.ada tests. I don't recall exactly
which ones failed, but you should definitely see a read off the end of
the source buffer.

Tom

[Pedro Alves]

Post by Joel Brobecker
Hi Tom,

Post by Tom Tromey
-fsanitize=address showed that ada-lang.c:move_bits can run off the
end of the source buffer. I believe this patch fixes the problem, by
arranging not to read from the source buffer once there are sufficient
bits in the accumulator.
gdb/ChangeLog
* ada-lang.c (move_bits): Don't run off the end of the source
buffer.

Thanks for the patch!
This is a part of the code that always forces me to think twice
(or ten times), each time I try to touch it. I should really start
adding comments to this code that detail what we are trying to do
as we do it.
I tested your change through our testsuite on the various baremetal
targets we have, and noticed that it causes regressions on ppc and arm
targets. It's hopefully something small, but just being back from
a holiday, I'm a bit tied up at work; I'll put that issue on my TODO
list to look at further.

I was going to suggest that this would benefit from unit tests in
the style of dwarf2read.c:copy_bitwise's, but, actually, isn't this
exactly the same as copy_bitwise? Can we get rid of ada-lang.c:move_bits?
(And maybe move copy_bitwise elsewhere?)

Thanks,
Pedro Alves

[Pedro Alves]

Post by Pedro Alves

Post by Joel Brobecker
Hi Tom,

Post by Tom Tromey
-fsanitize=address showed that ada-lang.c:move_bits can run off the
end of the source buffer. I believe this patch fixes the problem, by
arranging not to read from the source buffer once there are sufficient
bits in the accumulator.
gdb/ChangeLog
* ada-lang.c (move_bits): Don't run off the end of the source
buffer.

Thanks for the patch!
This is a part of the code that always forces me to think twice
(or ten times), each time I try to touch it. I should really start
adding comments to this code that detail what we are trying to do
as we do it.
I tested your change through our testsuite on the various baremetal
targets we have, and noticed that it causes regressions on ppc and arm
targets. It's hopefully something small, but just being back from
a holiday, I'm a bit tied up at work; I'll put that issue on my TODO
list to look at further.

I was going to suggest that this would benefit from unit tests in
the style of dwarf2read.c:copy_bitwise's, but, actually, isn't this
exactly the same as copy_bitwise? Can we get rid of ada-lang.c:move_bits?
(And maybe move copy_bitwise elsewhere?)

I meant to say dwarf2loc.c instead of dwarf2read.c.

Thanks,
Pedro Alves

[Joel Brobecker]

Post by Pedro Alves

Post by Pedro Alves
I was going to suggest that this would benefit from unit tests in
the style of dwarf2read.c:copy_bitwise's, but, actually, isn't this
exactly the same as copy_bitwise? Can we get rid of ada-lang.c:move_bits?
(And maybe move copy_bitwise elsewhere?)

I meant to say dwarf2loc.c instead of dwarf2read.c.

It does look exactly the same, doesn't it? I'll see if we can just
re-use dwarf2loc's copy_bitwise. Thanks for the suggestion!

[Joel Brobecker]

Hello,

Post by Joel Brobecker

Post by Pedro Alves

Post by Pedro Alves
I was going to suggest that this would benefit from unit tests in
the style of dwarf2read.c:copy_bitwise's, but, actually, isn't this
exactly the same as copy_bitwise? Can we get rid of ada-lang.c:move_bits?
(And maybe move copy_bitwise elsewhere?)

I meant to say dwarf2loc.c instead of dwarf2read.c.

It does look exactly the same, doesn't it? I'll see if we can just
re-use dwarf2loc's copy_bitwise. Thanks for the suggestion!

How about the attached? I ran it through AdaCore's testsuite on
all the platforms we support as well as the official testsuite on
x86_64-linux. No regression.

gdb/ChangeLog:

* ada-lang.c (move_bits): Delete. Update all callers to use
copy_bitwise instead.
* dwarf2loc.c (copy_bitwise, bits_to_str::bits_to_str)
(selftests::check_copy_bitwise, selftests::copy_bitwise_tests):
Move from here to utils.c.
(_initialize_dwarf2loc): Remove call to register copy_bitwise
selftests.
* utils.h (copy_bitwise): Add declaration.
* utils.c (copy_bitwise, bits_to_str::bits_to_str)
(selftests::check_copy_bitwise, selftests::copy_bitwise_tests):
Moved here from dwarf2loc.c.
(_initialize_utils): Register copy_bitwise selftests.

Thank you!

[Pedro Alves]

Post by Joel Brobecker
Hello,

Post by Joel Brobecker

Post by Pedro Alves

Post by Pedro Alves
I was going to suggest that this would benefit from unit tests in
the style of dwarf2read.c:copy_bitwise's, but, actually, isn't this
exactly the same as copy_bitwise? Can we get rid of ada-lang.c:move_bits?
(And maybe move copy_bitwise elsewhere?)

I meant to say dwarf2loc.c instead of dwarf2read.c.

It does look exactly the same, doesn't it? I'll see if we can just
re-use dwarf2loc's copy_bitwise. Thanks for the suggestion!

How about the attached? I ran it through AdaCore's testsuite on
all the platforms we support as well as the official testsuite on
x86_64-linux. No regression.
* ada-lang.c (move_bits): Delete. Update all callers to use
copy_bitwise instead.
* dwarf2loc.c (copy_bitwise, bits_to_str::bits_to_str)
Move from here to utils.c.
(_initialize_dwarf2loc): Remove call to register copy_bitwise
selftests.
* utils.h (copy_bitwise): Add declaration.
* utils.c (copy_bitwise, bits_to_str::bits_to_str)
Moved here from dwarf2loc.c.
(_initialize_utils): Register copy_bitwise selftests.
Thank you!
-- Joel

Great, thanks!

Nit, since the function is now public, I'd consider moving the unit
tests to under gdb/unittests/ instead, like, to a new
copy_bitwise-selftests.c file. (I'm mildly thinking that'd be a better
filename than utils-selftest.c because the function may well
move again in the future. Notice how gdb_realpath's unit tests
were left behind in gdb/utils.c even though gdb_realpath moved to
common/pathstuff.c.)

If you do that, you can drop the
'#if GDB_SELF_TEST' around the tests, since files in that
directory are not compiled if unit tests are disabled.

Regardless, LGTM.

Thanks,
Pedro Alves

[Joel Brobecker]

Post by Pedro Alves

Post by Joel Brobecker
* ada-lang.c (move_bits): Delete. Update all callers to use
copy_bitwise instead.
* dwarf2loc.c (copy_bitwise, bits_to_str::bits_to_str)
Move from here to utils.c.
(_initialize_dwarf2loc): Remove call to register copy_bitwise
selftests.
* utils.h (copy_bitwise): Add declaration.
* utils.c (copy_bitwise, bits_to_str::bits_to_str)
Moved here from dwarf2loc.c.
(_initialize_utils): Register copy_bitwise selftests.
Thank you!
-- Joel

Great, thanks!
Nit, since the function is now public, I'd consider moving the unit
tests to under gdb/unittests/ instead, like, to a new
copy_bitwise-selftests.c file. (I'm mildly thinking that'd be a better
filename than utils-selftest.c because the function may well
move again in the future. Notice how gdb_realpath's unit tests
were left behind in gdb/utils.c even though gdb_realpath moved to
common/pathstuff.c.)
If you do that, you can drop the
'#if GDB_SELF_TEST' around the tests, since files in that
directory are not compiled if unit tests are disabled.

I can do that. Since you said you're file reguardless, it's a little
easier for me to do it in two stages, so I'll go ahead and push this
one. I'll start on moving the unit tests again right after, and
will finish ASAP if it's not finished by end of today.

Thanks Pedro!

[Joel Brobecker]

Post by Joel Brobecker

Post by Pedro Alves
Nit, since the function is now public, I'd consider moving the unit
tests to under gdb/unittests/ instead, like, to a new
copy_bitwise-selftests.c file. (I'm mildly thinking that'd be a better
filename than utils-selftest.c because the function may well
move again in the future. Notice how gdb_realpath's unit tests
were left behind in gdb/utils.c even though gdb_realpath moved to
common/pathstuff.c.)
If you do that, you can drop the
'#if GDB_SELF_TEST' around the tests, since files in that
directory are not compiled if unit tests are disabled.

I can do that. Since you said you're file reguardless, it's a little
easier for me to do it in two stages, so I'll go ahead and push this
one. I'll start on moving the unit tests again right after, and
will finish ASAP if it's not finished by end of today.

Here it is:

gdb/ChangeLog:

* unittests/copy_bitwise-selftests.c: New file.
* utils.c (selftests::bits_to_str, selftests::check_copy_bitwise)
(selftests::copy_bitwise_tests): Delete, moving this code to
unittests/copy_bitwise-selftests.c instead.
(_initialize_utils): Do not register copy_bitwise tests.
* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
unittests/copy_bitwise-selftests.c.

Tested on x86_64-linux using the official testsuite, but also by
verifying that "maintenance selftests" still runs the copy_bitwise
tests.

OK to push to master?

Thank you!

[Pedro Alves]

Post by Joel Brobecker
* unittests/copy_bitwise-selftests.c: New file.
* utils.c (selftests::bits_to_str, selftests::check_copy_bitwise)
(selftests::copy_bitwise_tests): Delete, moving this code to
unittests/copy_bitwise-selftests.c instead.
(_initialize_utils): Do not register copy_bitwise tests.
* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
unittests/copy_bitwise-selftests.c.
Tested on x86_64-linux using the official testsuite, but also by
verifying that "maintenance selftests" still runs the copy_bitwise
tests.
OK to push to master?

OK.

Post by Joel Brobecker
Thank you!

Thanks,
Pedro Alves

[Joel Brobecker]

Post by Joel Brobecker
* unittests/copy_bitwise-selftests.c: New file.
* utils.c (selftests::bits_to_str, selftests::check_copy_bitwise)
(selftests::copy_bitwise_tests): Delete, moving this code to
unittests/copy_bitwise-selftests.c instead.
(_initialize_utils): Do not register copy_bitwise tests.
* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
unittests/copy_bitwise-selftests.c.
Tested on x86_64-linux using the official testsuite, but also by
verifying that "maintenance selftests" still runs the copy_bitwise
tests.
OK to push to master?

OK.

Thanks Pedro. Pushed to master. And thanks also to Tom, who pointed
out the issue and even tried to fix it; this started us towards
a nice little cleanup. Really nice :)