Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
stratakis mannequin opened this issue Oct 23, 2018 · 28 comments
Closed

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

stratakis mannequin opened this issue Oct 23, 2018 · 28 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir topic-XML type-bug An unexpected behavior, bug, or error

Comments

@stratakis
Copy link
Mannequin

stratakis mannequin commented Oct 23, 2018

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
    Copy link
    Mannequin Author

    stratakis mannequin commented Oct 23, 2018

    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.

    @stratakis stratakis mannequin added 3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir labels Oct 23, 2018
    @serhiy-storchaka
    Copy link
    Member

    Agree, this looks like a bug.

    Do you mind to create a PR Charalampos?

    @serhiy-storchaka serhiy-storchaka added easy topic-XML type-bug An unexpected behavior, bug, or error labels Oct 23, 2018
    @Shivank98
    Copy link
    Mannequin

    Shivank98 mannequin commented Oct 25, 2018

    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
    Copy link
    Mannequin Author

    stratakis mannequin commented Oct 25, 2018

    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
    Copy link
    Mannequin

    Shivank98 mannequin commented Oct 25, 2018

    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
    Copy link
    Mannequin

    Shivank98 mannequin commented Oct 25, 2018

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

    @serhiy-storchaka
    Copy link
    Member

    You need also to write a test.

    @Shivank98
    Copy link
    Mannequin

    Shivank98 mannequin commented Oct 25, 2018

    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
    Copy link
    Mannequin

    Shivank98 mannequin commented Oct 25, 2018

    I want to help in
    *need help in

    @taleinat
    Copy link
    Contributor

    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
    Copy link
    Mannequin

    Shivank98 mannequin commented Oct 25, 2018

    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
    Copy link
    Contributor

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

    @Shivank98
    Copy link
    Mannequin

    Shivank98 mannequin commented Oct 25, 2018

    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
    Copy link
    Contributor

    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
    Copy link
    Contributor

    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
    Copy link
    Contributor

    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
    Copy link
    Mannequin

    Shivank98 mannequin commented Oct 26, 2018

    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
    Copy link
    Contributor

    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
    Copy link
    Mannequin

    Shivank98 mannequin commented Oct 30, 2018

    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
    Copy link
    Mannequin

    Shivank98 mannequin commented Nov 19, 2018

    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
    Copy link
    Member

    encukou commented Nov 23, 2018

    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
    Copy link
    Member

    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
    Copy link
    Member

    I wrote a fix: PR 11061.

    @vstinner vstinner removed the easy label Dec 10, 2018
    @vstinner
    Copy link
    Member

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

    @vstinner
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir topic-XML type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants