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

[Enrico Granata]

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> 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>
> 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> 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
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20130221/f4dedd6e/attachment.html>