classification
Title: Coverity scan: copy/paste error in Lib/xml/dom/minidom.py
Type: behavior Stage: resolved
Components: Library (Lib), XML Versions: Python 3.8, Python 3.7, Python 3.6, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: cstratak, petr.viktorin, serhiy.storchaka, shivank98, taleinat, vstinner
Priority: normal Keywords: patch

Created on 2018-10-23 15:30 by cstratak, last changed 2018-12-10 10:58 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
reproducer.py petr.viktorin, 2018-11-23 09:45
Pull Requests
URL Status Linked Edit
PR 11061 merged vstinner, 2018-12-10 08:56
PR 11066 merged vstinner, 2018-12-10 10:16
PR 11067 merged vstinner, 2018-12-10 10:18
PR 11068 merged vstinner, 2018-12-10 10:21
Messages (28)
msg328321 - (view) Author: Charalampos Stratakis (cstratak) * Date: 2018-10-23 15:30
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?
# 1922|                   clone.entities._seq.append(entity)
# 1923|                   if hasattr(e, '_call_user_data_handler'):
# 1924|->                     e._call_user_data_handler(operation, n, entity)
# 1925|       else:
# 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.
msg328330 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-23 19:09
Agree, this looks like a bug.

Do you mind to create a PR Charalampos?
msg328431 - (view) Author: Shivank Gautam (shivank98) * Date: 2018-10-25 13:20
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.
msg328432 - (view) Author: Charalampos Stratakis (cstratak) * Date: 2018-10-25 13:26
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.
msg328433 - (view) Author: Shivank Gautam (shivank98) * Date: 2018-10-25 13:47
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)
msg328434 - (view) Author: Shivank Gautam (shivank98) * Date: 2018-10-25 13:51
*1924-->   n._call_user_data_handler(operation, n, notation)
msg328435 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-25 13:51
You need also to write a test.
msg328439 - (view) Author: Shivank Gautam (shivank98) * Date: 2018-10-25 14:15
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?
msg328440 - (view) Author: Shivank Gautam (shivank98) * Date: 2018-10-25 14:16
>I want to help in
*need help in
msg328467 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-10-25 19:09
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.
msg328479 - (view) Author: Shivank Gautam (shivank98) * Date: 2018-10-25 20:03
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)
msg328483 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-10-25 20:17
Shivank, your last comment is unclear. Are you asking a question or just reporting some progress?
msg328485 - (view) Author: Shivank Gautam (shivank98) * Date: 2018-10-25 20:27
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.
msg328608 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-10-26 21:45
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.
msg328609 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-10-26 21:46
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.
msg328611 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-10-26 21:49
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.
msg328612 - (view) Author: Shivank Gautam (shivank98) * Date: 2018-10-26 21:52
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?
msg328613 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-10-26 21:58
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.)
msg328906 - (view) Author: Shivank Gautam (shivank98) * Date: 2018-10-30 12:11
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 :)
msg330072 - (view) Author: Shivank Gautam (shivank98) * Date: 2018-11-19 04:10
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(#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.
msg330301 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2018-11-23 09:45
Ah, XML is such an overengineered format!

I&nbsp;usually live with the standard HTML entities &ndash; but it turns out you can define your own!

Here's a reproducer which shows how to do that.
msg331476 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-12-10 08:53
The bug was introduced in 2003 by:

commit 787354c3b99c9a0c5fdbdd33d29f58ef26df379f
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
msg331477 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-12-10 08:58
I wrote a fix: PR 11061.
msg331484 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-12-10 10:12
New changeset 8e0418688906206fe59bd26344320c0fc026849e by Victor Stinner in branch 'master':
bpo-35052: Fix handler on xml.dom.minidom.cloneNode() (GH-11061)
https://github.com/python/cpython/commit/8e0418688906206fe59bd26344320c0fc026849e
msg331490 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-12-10 10:53
New changeset 3fd975583b8e43d8dc23c83d699cd10b1fee6f7f by Victor Stinner in branch '3.6':
bpo-35052: Fix handler on xml.dom.minidom.cloneNode() (GH-11061) (GH-11067)
https://github.com/python/cpython/commit/3fd975583b8e43d8dc23c83d699cd10b1fee6f7f
msg331492 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-12-10 10:56
New changeset c3cc75134d41c6d436c21d3d315dc069b4826432 by Victor Stinner in branch '3.7':
bpo-35052: Fix handler on xml.dom.minidom.cloneNode() (GH-11061) (GH-11066)
https://github.com/python/cpython/commit/c3cc75134d41c6d436c21d3d315dc069b4826432
msg331493 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-12-10 10:56
New changeset cecf313d1ef4adc0ee5338dd0ca9be0b98302c87 by Victor Stinner in branch '2.7':
bpo-35052: Fix handler on xml.dom.minidom.cloneNode() (GH-11061) (GH-11068)
https://github.com/python/cpython/commit/cecf313d1ef4adc0ee5338dd0ca9be0b98302c87
msg331494 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-12-10 10:58
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(#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.
History
Date User Action Args
2018-12-10 10:58:45vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg331494

stage: patch review -> resolved
2018-12-10 10:56:56vstinnersetmessages: + msg331493
2018-12-10 10:56:50vstinnersetmessages: + msg331492
2018-12-10 10:53:13vstinnersetmessages: + msg331490
2018-12-10 10:21:57vstinnersetpull_requests: + pull_request10302
2018-12-10 10:18:01vstinnersetpull_requests: + pull_request10301
2018-12-10 10:16:08vstinnersetpull_requests: + pull_request10300
2018-12-10 10:12:55vstinnersetmessages: + msg331484
2018-12-10 08:58:18vstinnersetkeywords: - easy

messages: + msg331477
2018-12-10 08:56:11vstinnersetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request10295
2018-12-10 08:53:41vstinnersetmessages: + msg331476
2018-11-23 09:45:35petr.viktorinsetfiles: + reproducer.py
nosy: + petr.viktorin
messages: + msg330301

2018-11-19 04:10:42shivank98setmessages: + msg330072
2018-10-30 12:11:49shivank98setmessages: + msg328906
2018-10-26 21:58:09taleinatsetmessages: + msg328613
2018-10-26 21:52:36shivank98setmessages: + msg328612
2018-10-26 21:49:27taleinatsetmessages: + msg328611
2018-10-26 21:46:53taleinatsetmessages: + msg328609
2018-10-26 21:45:40taleinatsetmessages: + msg328608
2018-10-25 20:27:12shivank98setmessages: + msg328485
2018-10-25 20:17:53taleinatsetmessages: + msg328483
2018-10-25 20:03:08shivank98setmessages: + msg328479
2018-10-25 19:09:35taleinatsetnosy: + taleinat
messages: + msg328467
2018-10-25 14:16:10shivank98setmessages: + msg328440
2018-10-25 14:15:13shivank98setmessages: + msg328439
2018-10-25 13:51:53serhiy.storchakasetmessages: + msg328435
2018-10-25 13:51:01shivank98setmessages: + msg328434
2018-10-25 13:49:43vstinnersetnosy: + vstinner
2018-10-25 13:47:50shivank98setmessages: + msg328433
2018-10-25 13:26:49cstrataksetmessages: + msg328432
2018-10-25 13:20:35shivank98setnosy: + shivank98
messages: + msg328431
2018-10-23 19:09:43serhiy.storchakasettype: behavior
components: + XML

keywords: + easy
nosy: + serhiy.storchaka
messages: + msg328330
stage: needs patch
2018-10-23 15:30:54cstratakcreate