Opened 8 years ago

Closed 8 years ago

#1172 closed defect (fixed)

Stats show with multiple items crashes bindctl

Reported by: jreed Owned by: jelte
Priority: medium Milestone: Sprint-20120403
Component: ~bind-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 7.02 Internal?: no

Description

Stats show accepts a single optional item.

If have two (regardless if they exist or not), bindctl will crash:

> Stats show a b 
Traceback (most recent call last):
  File "/usr/pkg/lib/python3.1/cmd.py", line 213, in onecmd
    func = getattr(self, 'do_' + cmd)
AttributeError: 'BindCmdInterpreter' object has no attribute 'do_Stats'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/reed/opt/bind10/bin/bindctl", line 149, in <module>
    tool.run()
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 128, in run
    self.cmdloop()
  File "/usr/pkg/lib/python3.1/cmd.py", line 139, in cmdloop
    stop = self.onecmd(line)
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 452, in onecmd
    Cmd.onecmd(self, line)
  File "/usr/pkg/lib/python3.1/cmd.py", line 215, in onecmd
    return self.default(line)
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 418, in default
    self._parse_cmd(line)
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 555, in _parse_cmd
    self._validate_cmd(cmd)
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 383, in _validate_cmd
    param_spec = command_info.get_param_with_name(param_name).param_spec
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/moduleinfo.py", line 102, in get_param_with_name
    return self.params[param_name]
KeyError: 1



Subtickets

Change History (15)

comment:1 Changed 8 years ago by jreed

This is not specific to Stats show. It also happens with Stats remove or Stats set.

comment:2 Changed 8 years ago by jreed

Also related.... if I made a mistake like, I get:

> Stats set [ "foo": "bar" ]
Traceback (most recent call last):
  File "/usr/pkg/lib/python3.1/cmd.py", line 213, in onecmd
    func = getattr(self, 'do_' + cmd)
AttributeError: 'BindCmdInterpreter' object has no attribute 'do_Stats'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/reed/opt/bind10/bin/bindctl", line 149, in <module>
    tool.run()
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 128, in run
    self.cmdloop()
  File "/usr/pkg/lib/python3.1/cmd.py", line 139, in cmdloop
    stop = self.onecmd(line)
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 452, in onecmd
    Cmd.onecmd(self, line)
  File "/usr/pkg/lib/python3.1/cmd.py", line 215, in onecmd
    return self.default(line)
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 418, in default
    self._parse_cmd(line)
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 555, in _parse_cmd
    self._validate_cmd(cmd)
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 385, in _validate_cmd
    cmd.params[param_name] = isc.config.config_data.convert_type(param_spec, cmd.params[param_name])
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/isc/config/config_data.py", line 96, in convert_type
    map = ast.literal_eval(value)
  File "/usr/pkg/lib/python3.1/ast.py", line 49, in literal_eval
    node_or_string = parse(node_or_string, mode='eval')
  File "/usr/pkg/lib/python3.1/ast.py", line 37, in parse
    return compile(source, filename, mode, PyCF_ONLY_AST)
  File "<unknown>", line 1
    ["foo":"bar"]
          ^
SyntaxError: invalid syntax



comment:3 Changed 8 years ago by shane

  • Defect Severity changed from N/A to Medium
  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:4 Changed 8 years ago by jelte

  • Milestone set to Next-Sprint-Proposed

comment:5 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 4

comment:6 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120403

comment:7 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:8 Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

I couldn't reproduce this specific error; I did not get a crash, but an error message.

I could find one way to crash it in the interpreter, and have fixed that (first commit).

I also found that the error was quite incomprehensible, but that had more to do with the output of 'help'; which I have taken the liberty to change a little; it used to be split up into mandatory and optional arguments, without mentioning the order of the arguments (originally bindctl had only named command parameters :P)

So i have changed the 'help' output in two ways; it prints command arguments in their correct order, and it appends 'optional' or 'mandatory' to their type (which now no longer prints <type:> but just their type.

Renamed the getter method from get_name to the more useful get_basic_info.

Oh and I noticed the 'description' from the specfile never got set for command parameters, fixed that.

I know this could be considered out of scope usability stuff, but it was relatively easy output-only fixes that i think are very useful.

Example:

Where it used to say:

Command  set 	(set the value of specified name in statistics data)
		help (Get help for command)

Mandatory parameters:
    owner <type: string>

    data <type: map>


Optional parameters:
    pid <type: integer>

Note that 'data' is the *third* argument, and pid the (optional) second, which is unclear from the above. (on an unrelated note, I don't think Stats set should be a public command)

It now shows:

Command  set 	(set the value of specified name in statistics data)
		help (Get help for command)
Parameters:
    owner (string, mandatory)
        module name of the owner of the statistics data
    pid (integer, optional)
        process id of the owner module
    data (map, mandatory)
        statistics data set of the owner

Not sure if this needs a changelog; if so
[bug] Fixed a parser bug in bindctl that could make bindctl crash. Also improved 'command help' output; argument order is now shown correctly, and parameter descriptions are shown as well.

comment:9 follow-up: Changed 8 years ago by jreed

I also can no longer reproduce my crash issues related to this using recent version. (I am not sure why I said stats remove also had that issue as that command does not exist.)

Your new help output looks nicer. (I did not review the code.)

By the way, why is the "help (get help for command)" indented?

comment:10 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:11 in reply to: ↑ 9 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jelte

Hello

Replying to jelte:

I couldn't reproduce this specific error; I did not get a crash, but an error message.

I could find one way to crash it in the interpreter, and have fixed that (first commit).

So, congratulation for fixing a different issue than this ticket described. If I were a bureaucrat, I'd say you should have closed this ticket and created a new one instead, putting it to next sprint proposed, etc, etc…̣ :-P But as the new problem is solved, I think it is OK to keep.

Anyway, I believe it can't be reproduced because the Stats show got an extra argument recently and it acts differently. I can reproduce with very similar backtrace here:

> StatsHttpd status x y z
Traceback (most recent call last):
  File "/usr/lib64/python3.2/cmd.py", line 212, in onecmd
    func = getattr(self, 'do_' + cmd)
AttributeError: 'BindCmdInterpreter' object has no attribute 'do_StatsHttpd'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "./bin/bindctl", line 149, in <module>
    result = tool.run()
  File "/home/vorner/testing/bind10/lib64/python3.2/site-packages/bindctl/bindcmd.py", line 138, in run
    self.cmdloop()
  File "/usr/lib64/python3.2/cmd.py", line 138, in cmdloop
    stop = self.onecmd(line)
  File "/home/vorner/testing/bind10/lib64/python3.2/site-packages/bindctl/bindcmd.py", line 468, in onecmd
    Cmd.onecmd(self, line)
  File "/usr/lib64/python3.2/cmd.py", line 214, in onecmd
    return self.default(line)
  File "/home/vorner/testing/bind10/lib64/python3.2/site-packages/bindctl/bindcmd.py", line 434, in default
    self._parse_cmd(line)
  File "/home/vorner/testing/bind10/lib64/python3.2/site-packages/bindctl/bindcmd.py", line 570, in _parse_cmd
    self._validate_cmd(cmd)
  File "/home/vorner/testing/bind10/lib64/python3.2/site-packages/bindctl/bindcmd.py", line 368, in _validate_cmd
    params[param_count - 2] += params[param_count - 1]
KeyError: -1

I know this could be considered out of scope usability stuff, but it was relatively easy output-only fixes that i think are very useful.

OK, that looks nice. We need more of these output fixes, they make the software more sexy ;-).

Not sure if this needs a changelog; if so
[bug] Fixed a parser bug in bindctl that could make bindctl crash. Also improved 'command help' output; argument order is now shown correctly, and parameter descriptions are shown as well.

Maybe it'll be needed. I believe it can be the same after ↑ is fixed.

Replying to jreed:

I also can no longer reproduce my crash issues related to this using recent version. (I am not sure why I said stats remove also had that issue as that command does not exist.)

That command no longer exists, as it was removed recently. But it did exist. And I believe this is a problem of any command with only mandatory parameters.

comment:12 in reply to: ↑ 11 Changed 8 years ago by naokikambe

Replying to vorner:

That command no longer exists, as it was removed recently. But it did exist.

Yes, I removed it at this change.

291.    [func]          naokikambe
        Statistics items are specified by each module's spec file.
        Stats module can read these through the config manager. Stats
        module and stats httpd report statistics data and statistics
        schema by each module via both bindctl and HTTP/XML.
        (Trac #928,#929,#930,#1175,
        git 054699635affd9c9ecbe7a108d880829f3ba229e)

Statistics specification is managed under each spec file since that change.
I also removed the 'reset' command at that time.

Regards

comment:13 Changed 8 years ago by jelte

  • Owner changed from jelte to vorner

ah doh. Ok latest push also fixes that KeyError? (bad loop bound check)

comment:14 Changed 8 years ago by vorner

  • Owner changed from vorner to jelte
  • Total Hours changed from 0 to 1.02

Hello

OK, seems fine to merge.

comment:15 Changed 8 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 1.02 to 7.02

thanks! merged, closing ticket.

Note: See TracTickets for help on using tickets.