Fix for pr16196: Honor fetch limit for strings of known size

Fix for pr16196: Honor fetch limit for strings of known size

[Sterling Augustine]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

The enclosed patch fixes 16196, which was revealed when an unitialized
C++ string happened to point to valid memory and had a garbage in the
size field that happened to look very large. GDB then spins a very
long time reading invalid memory.

The patch fixes this by enforcing the fetch limit (which, in turn, is
set by print elements) even when the size of the string is known. This
makes the function's behavior similar to when the size of the string
isn't know.

I checked all callers, and it this does not cause any difference in
behavior, and reveals no new problems in the test-suite.

Thanks

Sterling

2013-11-22  Sterling Augustine  <saugustine@google.com>

     PR backtrace/16196:
     * valprint.c (read_string): Set new variable fetchlen based on
     fetchlimit and size.  Use it in call to partial_memory_read.
     Update comment.

[-- Attachment #2: honor-fetchlimit.patch --]
[-- Type: text/x-patch, Size: 1725 bytes --]

diff --git a/gdb/valprint.c b/gdb/valprint.c
index ea877f3..ecc3411 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -1757,11 +1757,13 @@ partial_memory_read (CORE_ADDR memaddr, gdb_byte *myaddr,
    free, and BYTES_READ will be set to the number of bytes read.  Returns 0 on
    success, or a target_xfer_error on failure.
 
-   If LEN > 0, reads exactly LEN characters (including eventual NULs in
-   the middle or end of the string).  If LEN is -1, stops at the first
-   null character (not necessarily the first null byte) up to a maximum
-   of FETCHLIMIT characters.  Set FETCHLIMIT to UINT_MAX to read as many
-   characters as possible from the string.
+   If LEN > 0, reads the lesser of LEN or FETCHLIMIT characters
+   (including eventual NULs in the middle or end of the string).
+
+   If LEN is -1, stops at the first null character (not necessarily
+   the first null byte) up to a maximum of FETCHLIMIT characters.  Set
+   FETCHLIMIT to UINT_MAX to read as many characters as possible from
+   the string.
 
    Unless an exception is thrown, BUFFER will always be allocated, even on
    failure.  In this case, some characters might have been read before the
@@ -1807,10 +1809,12 @@ read_string (CORE_ADDR addr, int len, int width, unsigned int fetchlimit,
 
   if (len > 0)
     {
-      *buffer = (gdb_byte *) xmalloc (len * width);
+      unsigned int fetchlen = min (len, fetchlimit);
+
+      *buffer = (gdb_byte *) xmalloc (fetchlen * width);
       bufptr = *buffer;
 
-      nfetch = partial_memory_read (addr, bufptr, len * width, &errcode)
+      nfetch = partial_memory_read (addr, bufptr, fetchlen * width, &errcode)
 	/ width;
       addr += nfetch * width;
       bufptr += nfetch * width;

Re: Fix for pr16196: Honor fetch limit for strings of known size

[Doug Evans]

On Fri, Nov 22, 2013 at 12:02 PM, Sterling Augustine
<saugustine@google.com> wrote:
> The enclosed patch fixes 16196, which was revealed when an unitialized
> C++ string happened to point to valid memory and had a garbage in the
> size field that happened to look very large. GDB then spins a very
> long time reading invalid memory.
>
> The patch fixes this by enforcing the fetch limit (which, in turn, is
> set by print elements) even when the size of the string is known. This
> makes the function's behavior similar to when the size of the string
> isn't know.
>
> I checked all callers, and it this does not cause any difference in
> behavior, and reveals no new problems in the test-suite.
>
> Thanks
>
> Sterling
>
> 2013-11-22  Sterling Augustine  <saugustine@google.com>
>
>      PR backtrace/16196:
>      * valprint.c (read_string): Set new variable fetchlen based on
>      fetchlimit and size.  Use it in call to partial_memory_read.
>      Update comment.

LGTM.
[There are still other issues, but this patch doesn't have to fix all of them.]

I'd change the "backtrace" in backtrace/16196 to something else, not
sure what though.
"gdb" is always a good fallback.  PR gdb/16196.

Re: Fix for pr16196: Honor fetch limit for strings of known size

[Sterling Augustine]

On Fri, Nov 22, 2013 at 12:35 PM, Doug Evans <dje@google.com> wrote:
> On Fri, Nov 22, 2013 at 12:02 PM, Sterling Augustine
> <saugustine@google.com> wrote:
>> The enclosed patch fixes 16196, which was revealed when an unitialized
>> C++ string happened to point to valid memory and had a garbage in the
>> size field that happened to look very large. GDB then spins a very
>> long time reading invalid memory.
>>
>> The patch fixes this by enforcing the fetch limit (which, in turn, is
>> set by print elements) even when the size of the string is known. This
>> makes the function's behavior similar to when the size of the string
>> isn't know.
>>
>> I checked all callers, and it this does not cause any difference in
>> behavior, and reveals no new problems in the test-suite.
>>
>> Thanks
>>
>> Sterling
>>
>> 2013-11-22  Sterling Augustine  <saugustine@google.com>
>>
>>      PR backtrace/16196:
>>      * valprint.c (read_string): Set new variable fetchlen based on
>>      fetchlimit and size.  Use it in call to partial_memory_read.
>>      Update comment.
>
> LGTM.
> [There are still other issues, but this patch doesn't have to fix all of them.]
>
> I'd change the "backtrace" in backtrace/16196 to something else, not
> sure what though.
> "gdb" is always a good fallback.  PR gdb/16196.


Checked in with the minor ChangeLog fix as requested.