Coverity scan: copy/paste error in Lib/xml/dom/minidom.py

[stratakis]

BPO	35052
Nosy	@vstinner, @taleinat, @encukou, @serhiy-storchaka, @stratakis, @shivank98
PRs	bpo-35052: Fix handler on xml.dom.minidom.cloneNode() #11061 [3.7] bpo-35052: Fix handler on xml.dom.minidom.cloneNode() (GH-11061) #11066 [3.6] bpo-35052: Fix handler on xml.dom.minidom.cloneNode() (GH-11061) #11067 [2.7] bpo-35052: Fix handler on xml.dom.minidom.cloneNode() (GH-11061) #11068
Files	reproducer.py

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-12-10.10:58:45.003>
created_at = <Date 2018-10-23.15:30:54.302>
labels = ['expert-XML', '3.8', 'type-bug', 'library', '3.7']
title = 'Coverity scan: copy/paste error in Lib/xml/dom/minidom.py'
updated_at = <Date 2018-12-10.10:58:45.002>
user = 'https://github.com/stratakis'

bugs.python.org fields:

activity = <Date 2018-12-10.10:58:45.002>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2018-12-10.10:58:45.003>
closer = 'vstinner'
components = ['Library (Lib)', 'XML']
creation = <Date 2018-10-23.15:30:54.302>
creator = 'cstratak'
dependencies = []
files = ['47942']
hgrepos = []
issue_num = 35052
keywords = ['patch']
message_count = 28.0
messages = ['328321', '328330', '328431', '328432', '328433', '328434', '328435', '328439', '328440', '328467', '328479', '328483', '328485', '328608', '328609', '328611', '328612', '328613', '328906', '330072', '330301', '331476', '331477', '331484', '331490', '331492', '331493', '331494']
nosy_count = 6.0
nosy_names = ['vstinner', 'taleinat', 'petr.viktorin', 'serhiy.storchaka', 'cstratak', 'shivank98']
pr_nums = ['11061', '11066', '11067', '11068']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue35052'
versions = ['Python 2.7', 'Python 3.6', 'Python 3.7', 'Python 3.8']

[stratakis]

Analyzing some coverity scan results I stumbled upon this issue:

Python-3.6.5/Lib/xml/dom/minidom.py:1914: original: "n._call_user_data_handler(operation, n, notation)" looks like the original copy.
Python-3.6.5/Lib/xml/dom/minidom.py:1924: copy_paste_error: "n" in "e._call_user_data_handler(operation, n, entity)" looks like a copy-paste error.
Python-3.6.5/Lib/xml/dom/minidom.py:1924: remediation: Should it say "e" instead?
\bpo-1922| clone.entities._seq.append(entity)
\bpo-1923| if hasattr(e, '_call_user_data_handler'):
\bpo-1924|-> e._call_user_data_handler(operation, n, entity)
\bpo-1925| else:
\bpo-1926| # Note the cloning of Document and DocumentType nodes is

It seems that the warning is indeed a bug, and the code in question was basically merged into python from pyxml 16 years ago.

[serhiy-storchaka]

Agree, this looks like a bug.

Do you mind to create a PR Charalampos?

[Shivank98]

Hey,
I am starting my journey with the contribution to CPython. and this is i think i can do these changes. if Charalampos is not there to work on it. i would like to try this.

[stratakis]

Hello Shivank. I had a PR ready locally which I was about to push, so you posted just at the right time :) Feel free to work on this issue.

[Shivank98]

Thanks, Charalampos :)
I would be thankful if you or Serhiy can help me a little bit.
so do I need to just do the following work?

1924--> n._call_user_data_handler(operation, n, entity)

[Shivank98]

*1924--> n._call_user_data_handler(operation, n, notation)

[serhiy-storchaka]

You need also to write a test.

[Shivank98]

Oh, I see, so I believe that's where my learning of CPython is going to start.
can anyone help me in completing this? what I understood till now is in minidom.py we need to change line 1924. else it, we need to create a test. I want to help in what should a test include and do we need to create a new function for it in test_minidom.py?

[Shivank98]

I want to help in
*need help in

[taleinat]

The test code should fail with the current, unfixed code, due to the bug described here. You will need to:

1. figure out which conditions will trigger the wrong behavior
2. set up a scenario where you can detect whether the behavior is correct
3. code this as a new test method in test_minidom.py
4. make the fix and ensure that the new test passes, as well as all other tests

After you've done this, you'll have something ready to be a PR. It will likely still require some work, e.g. cleaning up the code, adding/improving comments, conforming to our code style guidelines, and adding a NEWS entry. However, since this will be one of your first PRs, I suggest first just posting your test and fix as a PR.

Note that creating a PR will also trigger the test suite to be automatically run on several platforms; after ~15 minutes take a look at the results, and if there are any failures you should address them ASAP.

[Shivank98]

now when I am running test_minidom.py for both 1,2
1: e._call_user_data_handler(operation, n, entity)
2: n._call_user_data_handler(operation, n, notation)

I am receiving the same following result.

FAIL: testRemoveAttributeNode (main.MinidomTest)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "test_minidom.py", line 328, in testRemoveAttributeNode
    self.assertIs(node, child.removeAttributeNode(node))
AssertionError: <xml.dom.minidom.Attr object at 0x7fc62ab662a0> is not None

---

Ran 120 tests in 0.048s

FAILED (failures=1)

[taleinat]

Shivank, your last comment is unclear. Are you asking a question or just reporting some progress?

[Shivank98]

Oh Sorry, it was like was more like a question.
running test_minidom.py is giving an error for both (1 and 2), i am not sure even if "testRemoveAttributeNode (main.MinidomTest)" is related to _clone_node or not.

should i first try solve for this error? or maybe i haven't understood what type of test function i need to write.

[taleinat]

Shivank, there is currently technically no "error to solve", since we have no test that causes this erroneous behavior and catches it. We've only found what appears to be a bug by looking at the code, but we need to *verify* that it is indeed a bug. Also, this means that we have a piece of code that our test suite is missing; this is the real reason I insist that this needs a test.

You should trace what uses the affected code and which scenarios could lead to this bug causing unexpected behavior.

In this case, the code is in _clone_node(), which helpfully has the following comment in its doc-string: "Called by Node.cloneNode and Document.importNode". Work up from there, understand what this could affect, and think of a scenario where this code wouldn't work as expected.

[taleinat]

Looking at the code in Lib/xml/dom/minidom.py, this exact typo is also found in DocumentType.cloneNode(). That should be tested and fixed too.

[taleinat]

Shivank, I recommend taking a look at some of the existing tests for this module, found in Lib/test/test_minidom.py. See how they are built and how various functionality is tested. This should give you a good idea of what a test for this bug would look like.

[Shivank98]

I See, I need to find out for what test case _clone_node() will show unexpected behaviour, which will verify that it is a bug and we also need a test code to in test_minidom.py just to verify the behaviour.
am I going in right direction Tai?

[taleinat]

Shivank, indeed it seems you're now in the right direction.

A bit of clarification:

• The test shouldn't test _clone_node() directly, since that is an internal function. Rather, the test code should be as near as possible to what a user of the minidom module would reasonably write.
• Since this bug appears in two places, you'll likely need different tests for each of them. You should likely start with just one, and once you've successfully written a test for it and fixed it, move on to the other. (In the end, make each test+fix a separate commit in the PR.)

[Shivank98]

I just want to update that i was not able to work more in last two days as i was busy in some personal work. now i am on it and will update something soon. Sorry for delay :)

[Shivank98]

Hey Tal, I am extremely sorry for all delay. actually, due to internship and my university exams, I am unable to dedicate my time to bug(bpo-35052). please consider it and you can tell Charalampos Stratakis in the bug to push his prepared pull request.
I hope you will be still supportive when I will return in mid-dec after completing my exams.

[encukou]

Ah, XML is such an overengineered format!

I usually live with the standard HTML entities – but it turns out you can define your own!

Here's a reproducer which shows how to do that.

[vstinner]

The bug was introduced in 2003 by:

commit 787354c
Author: Martin v. Löwis <martin@v.loewis.de>
Date: Sat Jan 25 15:28:29 2003 +0000

Merge with PyXML 1.80:
Basic minidom changes to support the new higher-performance builder, as
described: http://mail.python.org/pipermail/xml-sig/2002-February/007217.html

[vstinner]

I wrote a fix: PR 11061.

[vstinner]

New changeset 8e04186 by Victor Stinner in branch 'master':
bpo-35052: Fix handler on xml.dom.minidom.cloneNode() (GH-11061)
8e04186

[vstinner]

New changeset 3fd9755 by Victor Stinner in branch '3.6':
bpo-35052: Fix handler on xml.dom.minidom.cloneNode() (GH-11061) (GH-11067)
3fd9755

[vstinner]

New changeset c3cc751 by Victor Stinner in branch '3.7':
bpo-35052: Fix handler on xml.dom.minidom.cloneNode() (GH-11061) (GH-11066)
c3cc751

[vstinner]

New changeset cecf313 by Victor Stinner in branch '2.7':
bpo-35052: Fix handler on xml.dom.minidom.cloneNode() (GH-11061) (GH-11068)
cecf313

[vstinner]

Thanks for the bug report and proposed fix Charalampos Stratakis, thanks for the reproducer script Petr Viktorin.

Shivank Gautam:

Hey Tal, I am extremely sorry for all delay. actually, due to internship and my university exams, I am unable to dedicate my time to bug(bpo-35052). please consider it and you can tell Charalampos Stratakis in the bug to push his prepared pull request. I hope you will be still supportive when I will return in mid-dec after completing my exams.

No problem. Charalampos Stratakis told me that he wanted to see this bug fixed to reduce the number of issues spotted by Coverity, so I fixed it.