Page MenuHomePhabricator

Get rid of the C-string parameter in DoExecute
ClosedPublic

Authored by teemperor on Jul 11 2018, 4:22 PM.

Details

Summary

This patch gets rid of the C-string parameter in the RawCommandObject::DoExecute function,
making the code simpler and less memory unsafe.

There seems to be a assumption in some command objects that this parameter could be a nullptr,
but from what I can see the rest of the API doesn't actually allow this (and other command
objects and related code pieces dereference this parameter without any checks).

Especially CommandObjectRegexCommand has error handling code for a nullptr that is now gone.

Diff Detail

Event Timeline

teemperor created this revision.Jul 11 2018, 4:22 PM
jingham requested changes to this revision.Jul 11 2018, 4:34 PM
jingham added a subscriber: jingham.

I think the CommandObjectRegex check that you took out wasn't something devious or clever, but was just an incomplete (and so buggy) version of the check:

if (command && command[0])

that you've been replacing with:

if (command.empty())

everywhere else in the patch. After all, the error message if you take the failing branch was:

result.AppendError("empty command passed to regular expression command");

So I think instead of taking this line out, you should also convert it to an if(command.empty()) and early return it with the error message you removed.

Other than that, this change looks fine.

This revision now requires changes to proceed.Jul 11 2018, 4:34 PM

@jingham I thought the same at first, but giving an error command.empty() turns out to be a problem. The command string contains the arguments (but not the command name itself). So when calling for example bt we would hit this error (as there are no args, so command is empty). I think this code was written with the assumption that command contains the whole command line or something like that.

Yeah, you are right about that. The command string presented to commands never contained the command name, that always got stripped off. But I don't think that ever left the command string to be nullptr... So you are right, that was just dead code.

teemperor accepted this revision.Jul 12 2018, 3:30 PM

Davide also approved it offline (thx).

This revision was not accepted when it landed; it landed in state Needs Revision.Jul 12 2018, 3:34 PM
This revision was automatically updated to reflect the committed changes.

I am debugging through it right now, but I believe this change caused several tests on Windows with Python 3 to fail. The failures are all related to UTF-8. For example:

UnicodeDecodeError: 'utf-8' codec can't decode byte 0x80 in position 0: invalid start byte

Sorry, I think I know why this is happening. Give me a bit to make a patch.