[Lldb-commits] [review request] fix bug that allows users to re-define commands

[Malea, Daniel]

Thanks, committed in r175798 (with a little slightly better testcase improvement).


On 2013-02-21, at 3:01 PM, Enrico Granata <egranata at apple.com<mailto:egranata at apple.com>>
 wrote:

Looks reasonable to me!

Thanks,
Enrico Granata
✉ egranata@.com
✆ 27683

On Feb 20, 2013, at 3:55 PM, "Malea, Daniel" <daniel.malea at intel.com<mailto:daniel.malea at intel.com>> wrote:

Thanks guys for the prompt feedback and explanation. The uninitialized member is worrisome…

Does the attached fix and test update look OK?


Dan

<fix_test_command_script_v2.patch>


On 2013-02-20, at 6:13 PM, Jim Ingham <jingham at apple.com<mailto:jingham at apple.com>>
wrote:

Ah, I understand now.  Yeah, the API is a little weird because you are passing a flag saying "Let me replace an extant command if the extant command agrees to be replaced." which seems a little over-specified.  But given that, Enrico is right and I was misreading the code.

Jim

On Feb 20, 2013, at 3:00 PM, Enrico Granata <egranata at apple.com<mailto:egranata at apple.com>> wrote:

Daniel,
I am pretty sure this test case used to pass. However, I did reproduce the failure you see.

As for fixing it, I am not sure passing "false" there would be the right thing. Doing so would make it impossible for users to replace their own scripted commands.
If you look at AddUserCommand(), it says:

      // do not allow replacement of internal commands
      if (CommandExists(name_cstr))
      {
          if (can_replace == false)
              return false;
          if (m_command_dict[name]->IsRemovable() == false)
              return false;
      }

Regardless of can_replace, a command can explicitly mark itself as non-removable, and that is the way system commands are supposed to be marked.
If you look at CommandObjectFrame, it is a MultiwordCommand, and for these:

  virtual bool
  IsRemovable() const { return m_can_be_removed; }

The *real* bug seems to be that m_can_be_removed is never initialized when the constructor runs.

My proposed fix (I have not prepared a patch for it because I am currently busy with a different task):
a) the constructor for CommandObjectMultiword should initialize m_can_be_removed - my guess is that the default value should be false (because that is also the default for CommandObject)
b) if necessary, individual multiword commands can change the value of the flag - or third-parties can call SetRemovable() - with the right value

As a side note, plugin multiword commands already get this right by calling SetRemovable(true) (SBCommandInterpeter.cpp  lines 416 and 470)

I will gladly appreciate any feedback on this proposal.
Assuming there are no objections, Daniel, feel free to write a patch to this effect, or if you are already busy, let me know and I will do this ASAP.

Thanks,
Enrico Granata
✉ egranata@.com
✆ 27683

On Feb 20, 2013, at 2:36 PM, "Malea, Daniel" <daniel.malea at intel.com<mailto:daniel.malea at intel.com>> wrote:

Hi,

I noticed the TestCommandScript.py test case is failing one line 118, where it verifies that a user cannot redefine a command named 'frame'.

I believe the failure is due to CommandObjectCommands.cpp passing the wrong value (true) for the 'can_replace' parameter to CommandInterpreter::AddUserCommand… When this is set to 'true', which allows the test-case to re-define the 'frame' command to be something else.

Attached patch should fix it. Any concerns/comments, or can I commit this as-is?

I'm still not exactly sure how CommandObjectCommands works, and why there's two code paths that end up calling AddUserCommand..so I figured I'd run it by the list first.


Dan


<fix_test_command_script.patch>


_______________________________________________
lldb-commits mailing list
lldb-commits at cs.uiuc.edu<mailto:lldb-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits