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

[Jim Ingham]

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> 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> 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
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits