Opened 8 years ago

Closed 8 years ago

#1515 closed defect (fixed)

crash bindctl with "config add Auth/datasources[0]/zones" twice

Reported by: jreed Owned by: vorner
Priority: high Milestone: Sprint-20120110
Component: ~bind-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: none
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

I did:

> config add Auth/datasources
> config show Auth/datasources
Auth/datasources[0]/type        ""      string  (default)
Auth/datasources[0]/class       "IN"    string  (default)
Auth/datasources[0]/zones       []      list    (default)
> config set Auth/datasources[0]/type "memory"
> config add Auth/datasources[0]/zones
> config add Auth/datasources[0]/zones
Traceback (most recent call last):
  File "/usr/local/lib/python3.1/cmd.py", line 213, in onecmd
    func = getattr(self, 'do_' + cmd)
AttributeError: 'BindCmdInterpreter' object has no attribute 'do_config'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/bin/bindctl", line 149, in <module>
    result = tool.run()
  File "/usr/local/lib/python3.1/site-packages/bindctl/bindcmd.py", line 138, in run
    self.cmdloop()
  File "/usr/local/lib/python3.1/cmd.py", line 139, in cmdloop
    stop = self.onecmd(line)
  File "/usr/local/lib/python3.1/site-packages/bindctl/bindcmd.py", line 466, in onecmd
    Cmd.onecmd(self, line)
  File "/usr/local/lib/python3.1/cmd.py", line 215, in onecmd
    return self.default(line)
  File "/usr/local/lib/python3.1/site-packages/bindctl/bindcmd.py", line 432, in default
    self._parse_cmd(line)
  File "/usr/local/lib/python3.1/site-packages/bindctl/bindcmd.py", line 569, in _parse_cmd
    self._handle_cmd(cmd)
  File "/usr/local/lib/python3.1/site-packages/bindctl/bindcmd.py", line 410, in _handle_cmd
    self.apply_config_cmd(cmd)
  File "/usr/local/lib/python3.1/site-packages/bindctl/bindcmd.py", line 677, in apply_config_cmd
    cmd.params.get('value_for_set'))
  File "/usr/local/lib/python3.1/site-packages/isc/config/ccsession.py", line 503, in add_value
    self._add_value_to_list(identifier, value, module_spec)
  File "/usr/local/lib/python3.1/site-packages/isc/config/ccsession.py", line 456, in _add_value_to_list
    + identifier)
TypeError: unsupported operand type(s) for +: 'dict' and 'str'

I purposely added twice as I planned to add two zones.

This is bind10-devel-20111128 installed from port on FreeBSD.

Subtickets

Change History (11)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 2

comment:2 Changed 8 years ago by jelte

  • Milestone changed from New Tasks to Sprint-20120110

comment:3 Changed 8 years ago by jelte

  • Priority changed from major to critical

comment:4 Changed 8 years ago by shane

  • Feature Depending on Ticket set to none

comment:5 Changed 8 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:6 Changed 8 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

Hello

It should be ready for review. The problem was it wanted to raise an exception, but part of the message wasn't converted to string (and it passed the tests because it was tested on lists containing strings only). Now it properly shows that the item is already in the list. I added a check to make sure such thing doesn't happen with named sets as well.

However, I'm not sure if this is the correct fix. What is the reason for not allowing a list to have two same items? Should we remove the check and allow them instead?

If not, this fix could have this changelog entry:

[bug]		vorner
The bindctl no longer crashes when a duplicate non-string item is added to
a list. An error is properly reported.

comment:7 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:8 follow-up: Changed 8 years ago by stephen

  • Owner changed from stephen to vorner

Reviewed commit fb6cd6d5a9cb93c5e2e6f274919c16c2348eead2

src/lib/python/config/tests/ccsession_test.py
test_add_remove_value: After "Spec2/item5" is added, is there any reason that the following "assertEqual()" test is repeated?

However, I'm not sure if this is the correct fix. What is the reason for not allowing a list to have two same items? Should we remove the check and allow them instead?

Would we be adding two identical items, or two references to the same item? I think the latter would be counter-intuitive for users if they subsequently change what they think is one instance and find the other one has changed as well.

If not, this fix could have this changelog entry:

I think a slight re-phrasing sounds better:

Fixed problem where bindctl crashed when a duplicate non-string item was
added to a list.  This error is now properly reported.

Otherwise all OK.

comment:9 in reply to: ↑ 8 Changed 8 years ago by vorner

  • Owner changed from vorner to stephen

Hello

Replying to stephen:

src/lib/python/config/tests/ccsession_test.py
test_add_remove_value: After "Spec2/item5" is added, is there any reason that the following "assertEqual()" test is repeated?

No, not really, except maybe as an artefact of my experiments with the test. I removed it.

However, I'm not sure if this is the correct fix. What is the reason for not allowing a list to have two same items? Should we remove the check and allow them instead?

Would we be adding two identical items, or two references to the same item? I think the latter would be counter-intuitive for users if they subsequently change what they think is one instance and find the other one has changed as well.

It would be adding two identical but independent items.

If not, this fix could have this changelog entry:

I think a slight re-phrasing sounds better:

Fixed problem where bindctl crashed when a duplicate non-string item was
added to a list.  This error is now properly reported.

OK

comment:10 Changed 8 years ago by stephen

  • Owner changed from stephen to vorner

All OK, please merge.

comment:11 Changed 8 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed

Thanks, merged, closing

Note: See TracTickets for help on using tickets.