debugger: Safer GDB/MI parsing and other fixes #202
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
strcpy() requires that the source and destination strings do not overlap, so fix the code to avoid this.
This fixes a leak, a possible NULL-pointer dereference and buffer overflows.
Including: * Fix parsing hex escapes; * Allow short forms for hex and octal escapes; * Remove some dead code; * Disable debugging prints; * Only recognize ASCII for special meanings.
Apparently GCC is not smart enough to understand that when the a NULL name is passed to `gdb_mi_result_foreach_matched` the `strcmp()` call cannot possibly happen. Work around the warnings it emits by adding an explicit (and useless) check on the `strcmp()` argument, which probably will be optimized out anyway.
This is likely to *NOT* be completely compatible with what the original code did. There are some explicitly different things that may either have been errors in the old code or changes in GDB output. The changes seem (basic testing) to work fine on Debian's GDB 7.7.1. NOTE: There is currently no conversion from the locale encoding to UTF-8. The GDB/MI module decodes the 7bit strings from GDB, but they probably encode locale-encoded data. This means this probably won't work reliably on non-UTF-8 locales without an appropriate conversion. "Appropriate conversion" yet has to be defined -- but as UTF-8 covers all of Unicode it probably is OK to convert back and forth even when not strictly required, it should at worse be useless.
Actually this reason isn't handled, and if it worked earlier is just that it would use the previous stop reason in this case. The updated code broke that as it always set the reason even when not handled, so restore the old version.
Use `g_snprintf()` instead of the unsafe `sprintf()`.
Fix the code for prompt to be locale-agnostic and actually work. This doesn't change anything however, because the default action for unknown record type is reporting a prompt, and a real prompt would trigger it.
|
I will postpone this again. Now to 1.27 due to lag of review. |
|
@frlan yeah, thanks. Though, in practice I'll probably just merge is as-is after 1.26, as the previous maintainer dropped and was alright with it, and I got positive response from a few people having tested it in the context of other reports. This said, if anyone wishes to take maintenance of this plugin and/or review this and/or test this is more than welcome. I'm not planning to maintain it for real in the long term. |
|
I'm fine with it. With having Devyn looking into complete debugging stack I've got a good feeling its not ending in a huge mess. |
frlan
added a commit
that referenced
this pull request
Nov 15, 2015
debugger: Safer GDB/MI parsing and other fixes
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Introduces a proper GDB/MI parser to replace the dangerous string manipulations. This part of the patchset is large and might have introduced issues I overlooked. It probably requires extensive testing.
Note that I only replaced the GDB/MI parsing code with something more generic and robust, the logic is (should be) still the same.
The rest of the changes are more trivial and should be straightforward.
If needed or requested, I can split the changes and/or squash some of the commits together. I did not do it because I'm not certain it's needed, and I prefer to do it last in case this PR leads to additional changes.
Beware that I'm not a real user of the debugger plugin, so I only performed limited testing -- I tried to use the most features I could find, both before and after my changes, but didn't use the plugin in any kind of daily basis.