This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: plistlib assumes dict_type is descendent of dict
Type: Stage:
Components: Library (Lib) Versions: Python 3.11
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: ronaldoussoren Nosy List: Behdad.Esfahbod, ned.deily, ronaldoussoren, serhiy.storchaka
Priority: normal Keywords:

Created on 2015-04-23 18:35 by Behdad.Esfahbod, last changed 2022-04-11 14:58 by admin.

Files
File name Uploaded Description Edit
issue-24040.txt ronaldoussoren, 2015-04-27 11:02 review
Messages (8)
msg241875 - (view) Author: Behdad Esfahbod (Behdad.Esfahbod) Date: 2015-04-23 18:35
Please replace instances of type({}) in plistlib.py with self._dict_type.
msg241895 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2015-04-23 23:05
The documentation does not explicitly state whether or not dict_type values have to be instances / subclasses of dict.  Can you give a code example, preferably something that could be added to Lib/test/test_plistlib.py, of a use case for something that is not a subclass of dict?   Some possible courses of action here: (1) Document that dict_type must be an instance or subclass of dict; (2) Change plistlib as you suggest to allow non-subclasses of dict; (3) Change plistlib to test for a subclass of collections.abc.MutableMapping; (4) other?
msg241897 - (view) Author: Behdad Esfahbod (Behdad.Esfahbod) Date: 2015-04-23 23:18
I don't have a valid use-case in mind.  I was reading the code and noticed this discrepancy.

(4) replace the isinstance(self.stack[-1], type({})) with not isinstance(self.stack[-1], type([])).
msg241898 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2015-04-23 23:40
Sorry, I don't understand your suggested (4).  If someone cares to provide a suggested patch (with appropriate tests), we could review it.  Otherwise, I would be inclined to close this issue as not an issue.  Ronald, any opinion?
msg241899 - (view) Author: Behdad Esfahbod (Behdad.Esfahbod) Date: 2015-04-23 23:53
The items on the stack are created in two ways: [], and self._dict_type().

Currently the code assumes that self._dict_type() returns an object that passes isinstance(..., type({})).  I suggested the following two ways to improve this check:

  - Replace with: isinstance(..., self._dict_type)

  - Replace with: not isinstance(..., type([]))

Feel free to close.  I reported because I saw room for improvement.  I don't /need/ the change myself.

Thanks.
msg241910 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2015-04-24 05:13
The test for type({}) is indeed wrong and should check for self._dict_type instead. However, there needs to be a test before that change is made. 

--
On the road, hence brief. 

Op 24 apr. 2015 om 01:40 heeft Ned Deily <report@bugs.python.org> het volgende geschreven:

> 
> Ned Deily added the comment:
> 
> Sorry, I don't understand your suggested (4).  If someone cares to provide a suggested patch (with appropriate tests), we could review it.  Otherwise, I would be inclined to close this issue as not an issue.  Ronald, any opinion?
> 
> ----------
> 
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue24040>
> _______________________________________
msg242110 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2015-04-27 11:02
To react to myself: checking for self.dict_type might break users that pass in a callable:

   x = plistlib.load(fp, dict_type=lambda:{})

As Behdad memtioned testing that the type isn't list would be better:

   if not isinstance(..., list):


That attached patch adds a testcase and removes replaces the test for type({}) for something better. 

Note: it also replaces ``type([])`` with ``list``, the latter is cleaner.
msg407171 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-11-27 21:08
The patch LGTM, and I have nothing to add. Ronald, do you mind to create a PR.
History
Date User Action Args
2022-04-11 14:58:16adminsetgithub: 68228
2021-11-27 21:08:18serhiy.storchakasetassignee: ronaldoussoren

messages: + msg407171
nosy: + serhiy.storchaka
2021-11-27 12:51:39iritkatrielsetversions: + Python 3.11, - Python 3.4
2015-04-27 11:02:07ronaldoussorensetfiles: + issue-24040.txt

messages: + msg242110
2015-04-24 05:14:00ronaldoussorensetmessages: + msg241910
2015-04-23 23:53:08Behdad.Esfahbodsetmessages: + msg241899
2015-04-23 23:40:12ned.deilysetmessages: + msg241898
2015-04-23 23:18:04Behdad.Esfahbodsetmessages: + msg241897
2015-04-23 23:05:42ned.deilysetnosy: + ronaldoussoren, ned.deily
messages: + msg241895
2015-04-23 18:35:08Behdad.Esfahbodcreate