This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] fix bug with command `printf "%s\n", $_as_string($pc)`


On 2017-02-25 14:11, Matthew Malcomson wrote:
On 2017-02-25 06:45, Matthew Malcomson wrote:
CHANGELOG:

2017-02-19  Matthew Malcomson <hardenedapple@gmail.com>

    * python/py-value.c (convert_value_from_python): Include NULL
terminator in result.
    testsuite/gdb.python/py-as-string.c,
testsuite/gdb.python/py-as-string.exp: Update tests
    to account for NULL terminator from python string values.
doc/gdb.texinfo ($trace_func): Mention this value can't be used with printf.

There is a ChangeLog in the doc and testsuite directories, so you should place these entries in the relevant ChangeLogs. Also, look at this section of the GDB wiki for more info on the proper format.


So I should include the changelog entry as part of the patch? (I just
sent it in the email based on how I read the CONTRIBUTE file)

You did it right, the changes only go in the actual ChangeLog files just before pushing the patch.

Just make sure to put each change in the relevant ChangeLog, the one "closest" to the change in the directory structure. For example, for you change, I would do:

gdb/ChangeLog:

	* python/py-value.c (convert_value_from_python): Consider terminating
	NULL byte in string length.

gdb/doc/ChangeLog:

	* gdb.texinfo (Convenience Variables for Tracepoints): Mention that
	trace_func should not be used with output and not printf.

gdb/testsuite/ChangeLog:

	* gdb.python/py-as-string.c (malloc): New function.
	* gdb.python/py-as-string.exp (test_as_string): Test $_as_string on
	a string with printf.
	* gdb.python/py-mi.exp: Adjust array length.

IIUC, the goal of overriding malloc is to ensure that the memory return by malloc is not all zeroes, which would potentially hide the bug? If that's right, you could instead write a wrapper for malloc instead of a replacement. The wrapper would memset the allocated buffer to 'x'es, for example. This way, it will be safer in case there are many calls to malloc or calls with size > 51.

See option #2 of this answer: http://stackoverflow.com/a/262481

Yes, that was the reason. I used this way because I read that gdb also
worked on non-POSIX systems (windows especially) and thought having a
working test on all systems would be preferred (though I didn't check
that all systems support the testing framework).
I believe that no other calls to malloc are made in the inferior for
this test, and that this program isn't used anywhere else, so this
limit of 51 bytes is never hit.
I agree this is a bug waiting to happen, so I can accept if the
alternate would be preferred, but I thought I'd mention my reasoning.

That's a good justification too, I'm ok with either.

+void *malloc(size_t size)

We try to respect the GNU/GDB coding style even in tests, so please put the return type on its own line and put a space after the function name:


My apologies

Apologies accepted :)

void *
malloc (size_t size)
{
  ...
}

+{
+    if (size > sizeof(arena))

Space after sizeof.

+        return NULL;
+
+    return arena;
+}

The indentation in C/C++ code sould be two spaces per indent, until you have 8 spaces, it then becomes a tab.

+
 static enum EnumType enum_valid = ENUM_VALUE_B;
 static enum EnumType enum_invalid = 20;

diff --git a/gdb/testsuite/gdb.python/py-as-string.exp
b/gdb/testsuite/gdb.python/py-as-string.exp
index 0c44d5f174..819442834c 100644
--- a/gdb/testsuite/gdb.python/py-as-string.exp
+++ b/gdb/testsuite/gdb.python/py-as-string.exp
@@ -35,6 +35,12 @@ proc test_as_string { } {
     gdb_test "p \$_as_string(2)" "\"2\""
     gdb_test "p \$_as_string(enum_valid)" "\"ENUM_VALUE_B\""
     gdb_test "p \$_as_string(enum_invalid)" "\"20\""
+ # Test that the NULL character is included in the returned value.
+    gdb_test "printf \"%s\\n\", \$_as_string(\"hi\")" "\"hi\""
+    # Quote once to define the string, and once for the regexp.
+    gdb_test "interpreter-exec mi '-var-create test *
\$_as_string(\"Hello\")'" \
+ "\\^done,name=\"test\",numchild=\"8\",value=\"\\\[8]\",type=\"char
\\\[8]\",has_more=\"0\""

Indent this with a leading tab.

If you want to avoid massive escaping, you can use {} strings instead of "" strings. {} strings are treated literally, so there's no $variable substitution, no [proc invocation], no need to escape a literal backslash, etc. You still need to escape characters that have a special meaning in regex though.

"\\^done,name=\"test\",numchild=\"8\",value=\"\\\[8]\",type=\"char \\\[8]\",has_more=\"0\""

would become (I think, I have not tested)

{\^done,name="test",numchild="8",value="\[8]",type="char \[8]",has_more="0"}



Yes, this does work, I had chosen "" strings to match the previous
lines (I figured I'd have a comment either mentioning why this string
used different delimiters or why there was extra backslashing, and it
looked neater to me this way).
Would you prefer using {} strings? or was that just a heads-up in case
I didn't know?

It was just a heads up, since most people are not very literate in TCL (me included), feel free to use whichever you want.

Thanks,

Simon


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]