* [PATCH] fix bug with command `printf "%s\n", $_as_string($pc)` @ 2017-02-19 17:32 Matthew Malcomson 2017-02-20 18:20 ` Simon Marchi 0 siblings, 1 reply; 8+ messages in thread From: Matthew Malcomson @ 2017-02-19 17:32 UTC (permalink / raw) To: gdb-patches Automatic conversion of python string to gdb value leaves off the NULL byte. This doesn't matter with `print $_as_string($pc)` because the print command uses `value_print()` which limits how many characters are printed based on the length of the array. The command `printf "%s\n", $_as_string($pc)` instead writes the value into the inferior with `value_coerce_to_target()` and then treats the string as a NULL terminated string in the inferior. As the value created from `$_as_string($pc)` is not NULL terminated, this sometimes ends up printing extra characters (depending on where the inferior allocated the memory for this string). `printf "%s\n", "Some string"` doesn't have this problem because the gdb value type created here includes the NULL terminating byte, as is ensured in `evaluate_subexp_c()` file and line c-lang.c:677 (at the time of writing this), commented as `/* Write the terminating character. */`. There are two other places where a terminating NULL byte is not included when calling `value_cstring()`. When converting from a guile string into a gdb internal value structure. This doesn't cause any observable bugs because the string can't be stored in a variable (as there's no equivalent of the python `gdb.Function` class) and hence can't be passed to the `printf` command. There appears to have been some thought about terminating NULL bytes commented above `gdbscm_scm_to_string()`. Given there are no observable bugs and there has already been some thought about the NULL byte in this area I've not changed anything there. In `value_of_internalvar()` when the variable is of kind `INTERNALVAR_STRING`. There are only two internal variables of this kind, `$trace_func` and `$trace_file`. The info documentation says that `$trace_file` is not suitable for use in `printf`, and I believe the same should be said about `$trace_func`. Both are prohibited by the check on `get_traceframe_number()` in `call_function_by_hand_dummy()` that is called from `value_coerce_to_target()` during printf on string variables. I have made a slight change to the documentation here. ------------------------------------------------ CHANGELOG: 2017-02-19 Matthew Malcomson <hardenedapple@gmail.com> * python/py-value.c (convert_value_from_python): Include NULL terminator in result. doc/gdb.texinfo ($trace_func): Mention this value can't be used with printf. ------------------------------------------------- PATCH: commit 5a0ceb374621de8eb2264c6b9ae92cf936a66f6b Author: Matthew Malcomson <hardenedapple@gmail.com> Date: Sun Feb 19 14:35:09 2017 +0000 convert_value_from_python include terminating NULL When converting python strings to internal gdb Value strings, the NULL byte was initially left out, this can result in extra data from the inferior being printed when the resulting value is used with printf "%s\n", value diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index c465dc2f9f..5fb34853f1 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -13645,8 +13645,8 @@ The source file for the current trace snapshot. The name of the function containing @code{$tracepoint}. @end table -Note: @code{$trace_file} is not suitable for use in @code{printf}, -use @code{output} instead. +Note: @code{$trace_file} and @code{$trace_func} are not suitable for use in +@code{printf}, use @code{output} instead. Here's a simple example of using these convenience variables for stepping through all the trace snapshots and printing some of their diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c index eb3d307b19..c786f68865 100644 --- a/gdb/python/py-value.c +++ b/gdb/python/py-value.c @@ -1615,7 +1615,7 @@ convert_value_from_python (PyObject *obj) gdb::unique_xmalloc_ptr<char> s = python_string_to_target_string (obj); if (s != NULL) - value = value_cstring (s.get (), strlen (s.get ()), + value = value_cstring (s.get (), strlen (s.get ()) + 1, builtin_type_pychar); } else if (PyObject_TypeCheck (obj, &value_object_type)) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix bug with command `printf "%s\n", $_as_string($pc)` 2017-02-19 17:32 [PATCH] fix bug with command `printf "%s\n", $_as_string($pc)` Matthew Malcomson @ 2017-02-20 18:20 ` Simon Marchi 2017-02-25 11:45 ` Matthew Malcomson 0 siblings, 1 reply; 8+ messages in thread From: Simon Marchi @ 2017-02-20 18:20 UTC (permalink / raw) To: Matthew Malcomson; +Cc: gdb-patches On 2017-02-19 12:32, Matthew Malcomson wrote: > diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c > index eb3d307b19..c786f68865 100644 > --- a/gdb/python/py-value.c > +++ b/gdb/python/py-value.c > @@ -1615,7 +1615,7 @@ convert_value_from_python (PyObject *obj) > gdb::unique_xmalloc_ptr<char> s > = python_string_to_target_string (obj); > if (s != NULL) > - value = value_cstring (s.get (), strlen (s.get ()), > + value = value_cstring (s.get (), strlen (s.get ()) + 1, > builtin_type_pychar); > } > else if (PyObject_TypeCheck (obj, &value_object_type)) This fix looks good to me. One test (py-mi.exp) needs to be updated though. You can run all the Python-related tests using: $ make check TESTS="gdb.python/*.exp" Normally, the Python tests all pass reliably, unlike some other parts of the testsuite. It might also be good to improve gdb.python/py-as-string.exp to include a test for this bug. Thanks, Simon ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix bug with command `printf "%s\n", $_as_string($pc)` 2017-02-20 18:20 ` Simon Marchi @ 2017-02-25 11:45 ` Matthew Malcomson 2017-02-25 18:33 ` Simon Marchi 0 siblings, 1 reply; 8+ messages in thread From: Matthew Malcomson @ 2017-02-25 11:45 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches On 20/02/17 18:19, Simon Marchi wrote: > On 2017-02-19 12:32, Matthew Malcomson wrote: >> diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c >> index eb3d307b19..c786f68865 100644 >> --- a/gdb/python/py-value.c >> +++ b/gdb/python/py-value.c >> @@ -1615,7 +1615,7 @@ convert_value_from_python (PyObject *obj) >> gdb::unique_xmalloc_ptr<char> s >> = python_string_to_target_string (obj); >> if (s != NULL) >> - value = value_cstring (s.get (), strlen (s.get ()), >> + value = value_cstring (s.get (), strlen (s.get ()) + 1, >> builtin_type_pychar); >> } >> else if (PyObject_TypeCheck (obj, &value_object_type)) > > This fix looks good to me. > > One test (py-mi.exp) needs to be updated though. You can run all the > Python-related tests using: > > $ make check TESTS="gdb.python/*.exp" > > Normally, the Python tests all pass reliably, unlike some other parts > of the testsuite. > > It might also be good to improve gdb.python/py-as-string.exp to > include a test for this bug. > > Thanks, > > Simon Thanks -- I had mistakenly ignored the py-mi.exp failure assuming it was nothing to do with me (my apologies). I've included the test fixes you suggested below. ------------------------------------------------ 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. ------------------------------------------------- PATCH: commit 44fd8bd7af5cf4c6b32846dd78ebfecb7b8d9fa5 Author: Matthew Malcomson <hardenedapple@gmail.com> Date: Sun Feb 19 14:35:09 2017 +0000 convert_value_from_python include terminating NULL When converting python strings to internal gdb Value strings, the NULL byte was initially left out, this can result in extra data from the inferior being printed when the resulting value is used with printf "%s\n", value diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index c465dc2f9f..5fb34853f1 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -13645,8 +13645,8 @@ The source file for the current trace snapshot. The name of the function containing @code{$tracepoint}. @end table -Note: @code{$trace_file} is not suitable for use in @code{printf}, -use @code{output} instead. +Note: @code{$trace_file} and @code{$trace_file} are not suitable for use in +@code{printf}, use @code{output} instead. Here's a simple example of using these convenience variables for stepping through all the trace snapshots and printing some of their diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c index eb3d307b19..c786f68865 100644 --- a/gdb/python/py-value.c +++ b/gdb/python/py-value.c @@ -1615,7 +1615,7 @@ convert_value_from_python (PyObject *obj) gdb::unique_xmalloc_ptr<char> s = python_string_to_target_string (obj); if (s != NULL) - value = value_cstring (s.get (), strlen (s.get ()), + value = value_cstring (s.get (), strlen (s.get ()) + 1, builtin_type_pychar); } else if (PyObject_TypeCheck (obj, &value_object_type)) diff --git a/gdb/testsuite/gdb.python/py-as-string.c b/gdb/testsuite/gdb.python/py-as-string.c index de2e8a1951..c35a692712 100644 --- a/gdb/testsuite/gdb.python/py-as-string.c +++ b/gdb/testsuite/gdb.python/py-as-string.c @@ -15,6 +15,8 @@ You should have received a copy of the GNU General Public License along with this program. If not, see <http://www.gnu.org/licenses/>. */ +#include <stddef.h> + enum EnumType { ENUM_VALUE_A, ENUM_VALUE_B, @@ -22,6 +24,19 @@ enum EnumType { ENUM_VALUE_D, }; +static char arena[51] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"; + +/* Override malloc() so value_coerce_to_target() gets a known pointer, and we + know we'll see an error if $_as_string() returns a string that isn't NULL + terminated. */ +void *malloc(size_t size) +{ + if (size > sizeof(arena)) + return NULL; + + return arena; +} + 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\"" + gdb_test "interpreter-exec mi '-var-delete test'" "\\^done,ndeleted=\"1\"" } test_as_string diff --git a/gdb/testsuite/gdb.python/py-mi.exp b/gdb/testsuite/gdb.python/py-mi.exp index 736dc7a0d6..a5ad3f0f44 100644 --- a/gdb/testsuite/gdb.python/py-mi.exp +++ b/gdb/testsuite/gdb.python/py-mi.exp @@ -281,7 +281,7 @@ mi_create_dynamic_varobj nstype2 nstype2 1 \ "create nstype2 varobj" mi_list_varobj_children nstype2 { - { {nstype2.<error at 0>} {<error at 0>} 6 {char \[6\]} } + { {nstype2.<error at 0>} {<error at 0>} 7 {char \[7\]} } } "list children after setting exception flag" mi_create_varobj me me \ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix bug with command `printf "%s\n", $_as_string($pc)` 2017-02-25 11:45 ` Matthew Malcomson @ 2017-02-25 18:33 ` Simon Marchi 2017-02-25 19:11 ` Matthew Malcomson 0 siblings, 1 reply; 8+ messages in thread From: Simon Marchi @ 2017-02-25 18:33 UTC (permalink / raw) To: Matthew Malcomson; +Cc: gdb-patches Hi Matthew, I noted mostly some minor formatting issues, in general it looks good to me. One comment about malloc. 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. https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_Formatted_GNU_ChangeLog > +static char arena[51] = > "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"; > + > +/* Override malloc() so value_coerce_to_target() gets a known pointer, > and we > + know we'll see an error if $_as_string() returns a string that > isn't NULL > + terminated. */ 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 > +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: 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"} Finally, feel free to add newlines between logical groups of lines to make the code more readable. Thanks, Simon ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix bug with command `printf "%s\n", $_as_string($pc)` 2017-02-25 18:33 ` Simon Marchi @ 2017-02-25 19:11 ` Matthew Malcomson 2017-02-25 21:33 ` Simon Marchi 0 siblings, 1 reply; 8+ messages in thread From: Matthew Malcomson @ 2017-02-25 19:11 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches On 25/02/17 18:33, Simon Marchi wrote: > Hi Matthew, > > I noted mostly some minor formatting issues, in general it looks good > to me. One comment about malloc. Sure, I have just a few questions > 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) > > https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_Formatted_GNU_ChangeLog > > >> +static char arena[51] = >> "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"; >> + >> +/* Override malloc() so value_coerce_to_target() gets a known >> pointer, and we >> + know we'll see an error if $_as_string() returns a string that >> isn't NULL >> + terminated. */ > > 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. >> +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 > 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? > > Finally, feel free to add newlines between logical groups of lines to > make the code more readable. > > Thanks, > > Simon > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix bug with command `printf "%s\n", $_as_string($pc)` 2017-02-25 19:11 ` Matthew Malcomson @ 2017-02-25 21:33 ` Simon Marchi 2017-02-26 13:20 ` Matthew Malcomson 0 siblings, 1 reply; 8+ messages in thread From: Simon Marchi @ 2017-02-25 21:33 UTC (permalink / raw) To: Matthew Malcomson; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix bug with command `printf "%s\n", $_as_string($pc)` 2017-02-25 21:33 ` Simon Marchi @ 2017-02-26 13:20 ` Matthew Malcomson 2017-02-26 14:44 ` Simon Marchi 0 siblings, 1 reply; 8+ messages in thread From: Matthew Malcomson @ 2017-02-26 13:20 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches [-- Attachment #1: Type: text/plain, Size: 2030 bytes --] I've attached the patch with correct formatting because my email client replaces tabs with spaces. I'll leave the changelog entries as you suggested. Thanks again, Matthew > 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. > > [-- Attachment #2: formatted_patch.patch --] [-- Type: text/x-patch, Size: 4031 bytes --] commit 28312c70fcba81ef50a93ff52dde47230efc35cb Author: Matthew Malcomson <hardenedapple@gmail.com> Date: Sun Feb 26 13:10:09 2017 +0000 convert_value_from_python include terminating NULL When converting python strings to internal gdb Value strings, the NULL byte was initially left out, this can result in extra data from the inferior being printed when the resulting value is used with printf "%s\n", value diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index 962325be3a..486b7899fb 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -13645,8 +13645,8 @@ The source file for the current trace snapshot. The name of the function containing @code{$tracepoint}. @end table -Note: @code{$trace_file} is not suitable for use in @code{printf}, -use @code{output} instead. +Note: @code{$trace_file} and @code{$trace_file} are not suitable for use in +@code{printf}, use @code{output} instead. Here's a simple example of using these convenience variables for stepping through all the trace snapshots and printing some of their diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c index eb3d307b19..c786f68865 100644 --- a/gdb/python/py-value.c +++ b/gdb/python/py-value.c @@ -1615,7 +1615,7 @@ convert_value_from_python (PyObject *obj) gdb::unique_xmalloc_ptr<char> s = python_string_to_target_string (obj); if (s != NULL) - value = value_cstring (s.get (), strlen (s.get ()), + value = value_cstring (s.get (), strlen (s.get ()) + 1, builtin_type_pychar); } else if (PyObject_TypeCheck (obj, &value_object_type)) diff --git a/gdb/testsuite/gdb.python/py-as-string.c b/gdb/testsuite/gdb.python/py-as-string.c index de2e8a1951..e53f3a9b64 100644 --- a/gdb/testsuite/gdb.python/py-as-string.c +++ b/gdb/testsuite/gdb.python/py-as-string.c @@ -15,6 +15,8 @@ You should have received a copy of the GNU General Public License along with this program. If not, see <http://www.gnu.org/licenses/>. */ +#include <stddef.h> + enum EnumType { ENUM_VALUE_A, ENUM_VALUE_B, @@ -22,6 +24,20 @@ enum EnumType { ENUM_VALUE_D, }; +static char arena[51] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"; + +/* Override malloc() so value_coerce_to_target() gets a known pointer, and we + know we'll see an error if $_as_string() returns a string that isn't NULL + terminated. */ +void +*malloc (size_t size) +{ + if (size > sizeof (arena)) + return NULL; + + return arena; +} + 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..e4625631c2 100644 --- a/gdb/testsuite/gdb.python/py-as-string.exp +++ b/gdb/testsuite/gdb.python/py-as-string.exp @@ -35,6 +35,13 @@ 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\"" + gdb_test "interpreter-exec mi '-var-delete test'" "\\^done,ndeleted=\"1\"" } test_as_string diff --git a/gdb/testsuite/gdb.python/py-mi.exp b/gdb/testsuite/gdb.python/py-mi.exp index 736dc7a0d6..a5ad3f0f44 100644 --- a/gdb/testsuite/gdb.python/py-mi.exp +++ b/gdb/testsuite/gdb.python/py-mi.exp @@ -281,7 +281,7 @@ mi_create_dynamic_varobj nstype2 nstype2 1 \ "create nstype2 varobj" mi_list_varobj_children nstype2 { - { {nstype2.<error at 0>} {<error at 0>} 6 {char \[6\]} } + { {nstype2.<error at 0>} {<error at 0>} 7 {char \[7\]} } } "list children after setting exception flag" mi_create_varobj me me \ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix bug with command `printf "%s\n", $_as_string($pc)` 2017-02-26 13:20 ` Matthew Malcomson @ 2017-02-26 14:44 ` Simon Marchi 0 siblings, 0 replies; 8+ messages in thread From: Simon Marchi @ 2017-02-26 14:44 UTC (permalink / raw) To: Matthew Malcomson; +Cc: gdb-patches On 2017-02-26 08:20, Matthew Malcomson wrote: > I've attached the patch with correct formatting because my email > client replaces tabs with spaces. I'll leave the changelog entries as > you suggested. Just a heads up for others, the patch does not apply for me, I think because it has CRLF line terminators. When I convert it to LF it applies fine. Just one comment: void *malloc (size_t size) should be: void * malloc (size_t size) Otherwise, the patch looks good to me. Now you just have to wait until somebody with actual authority looks at it :). ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-02-26 14:44 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-19 17:32 [PATCH] fix bug with command `printf "%s\n", $_as_string($pc)` Matthew Malcomson 2017-02-20 18:20 ` Simon Marchi 2017-02-25 11:45 ` Matthew Malcomson 2017-02-25 18:33 ` Simon Marchi 2017-02-25 19:11 ` Matthew Malcomson 2017-02-25 21:33 ` Simon Marchi 2017-02-26 13:20 ` Matthew Malcomson 2017-02-26 14:44 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).