classification
Title: Add tests for NUL checking in certain strs
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, alex, amaury.forgeotdarc, berker.peksag, hynek, loewis, martin.panter, pitrou, python-dev, r.david.murray, terry.reedy, vstinner
Priority: normal Keywords: patch

Created on 2012-01-24 02:05 by alex, last changed 2016-07-23 13:20 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
issue13849.diff berker.peksag, 2016-07-21 19:20 review
Messages (24)
msg151877 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2012-01-24 02:05
ATM there's no tests (at least in 2.x, I haven't checked 3.x yet) for this behavior:

>>> os.path.exists("/tmp\x00abcds")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.2/genericpath.py", line 18, in exists
    os.stat(path)
TypeError: embedded NUL character
msg151878 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-01-24 02:23
Why is that a bug? There is no feature in Python saying that the test suite covers the code fully (by some kind of measurement).

New tests should only be added to 3.3, unless they test for a newly-fixed bug (and even then the test may not be backported to the maintenance release).
msg151879 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-01-24 02:42
Adding tests helps the other VMs, which generally are trailing behind the CPython releases.
msg151883 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-01-24 08:12
If other VMs need this test for some reason, they can easily add a test case themselves. I'm -1 on adding test cases to bug fix releases just for completeness. A lacking test is not a bug, and hence must not be added to a bug fix release.
msg151903 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-01-24 13:21
My understanding (and recollection, but I don't have notes I can point at to hand) is that one goal that arose from recent VM and language summits was for the CPython test suite to be used as the validating test suite, with CPython-specific tests marked as such so that the other VMs can easily "not conform" to those tests.  Thus my comment that it is helpful to the other VMs to add tests.
msg151904 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-01-24 13:25
Oh, and in case it isn't clear, this request is *coming* from one of the other VMs (pypy), so if my summit recollection is correct, they are in fact "adding a test that they need" by submitting this issue :)

(Or at least they will have once Alex posts the patch.)
msg151907 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-01-24 13:46
If so, I think this change should not checked into the 2.7 branch. Instead, a separate branch should be made for changes not intended for CPython, but for Python implementations in general. Making the Python test suite usable for other implementations is certainly not a feature of Python 2.7.
msg151908 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-01-24 13:49
Reconsidering: I think it shouldn't be checked into the cpython *repository*. Instead, if PyPy developers want to contribute changes to the test suite and standard library to improve the standard library, there should be a separate Mercurial repository for that, and PyPy developers might get a blanket write permission to add changes unless they explicitly feel a need for review.
msg152140 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2012-01-27 23:26
This is a bit related to #13848 files 2 seconds earlier.
msg152141 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-01-27 23:35
Well, adding tests is certainly useful to avoid regressions in CPython (not only PyPy or Jython). It seems to me that the only reasonable discussion is whether they should be committed to 2.7 and 3.2, or only the default branch.

(I personally like adding tests to stable branches as well, since that avoids potential regressions in further bugfix releases)

If we had had such tests from the beginning, 3.x wouldn't have regressed in that matter (see #13848).
msg153436 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2012-02-15 21:13
See also issue #13617.
msg160847 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-05-16 12:28
Why is this one still open? I'm afraid the questions raised aren't fit to be discussed in a ticket (weren't the outsourcing of of stdlib an item at the language summit?).

I tend to close it as rejected (although I rather disagree) unless someone has compelling reasons to keep it lingering.
msg160863 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-16 13:44
> Why is this one still open? I'm afraid the questions raised aren't
> fit to be discussed in a ticket

The original request (add tests for NUL character checking) is still relevant AFAIK. All it needs is a patch - by Alex or anybody else :-)
msg160891 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2012-05-16 17:20
I'm perfectly happy to write a patch for this, the only reason I didn't was Martin and others expressed opposition to committing it.  If there's a philosophicaly opposition to the patch I won't write it :)
msg160925 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2012-05-16 19:53
There are two issues. 1) what is the requested changed and is it technically correct; 2) if correct, should it be made from a policy viewpoint.

1) Alex, do I understand correctly that you see the behavior of disallowing nulls in paths as correct, and that you want an AssertRaises(..., TypeError) test added?

This rule seems like an OS rule, not a Python language rule. Is it uniform across all OSes?

The following is near the top of the 3.3 os module doc:

"Note: All functions in this module raise OSError in the case of invalid or inaccessible file names and paths, or other arguments that have the correct type, but are not accepted by the operating system."

On that basis, I would expect OSError. But perhaps the doc needs revision to accord with the reworking of OSErrors. On 3.3, Win7, I actually get

>>> os.stat("/tmp\x00abcds")
Traceback (most recent call last):
  File "<pyshell#3>", line 1, in <module>
    os.stat("/tmp\x00abcds")
FileNotFoundError: [Error 2] The system cannot find the file specified: '/tmp\x00abcds'

It appears that os.stat did not check for embedded nulls but just passed the string to Windows.


2. I am not sure if I understand the opposition to adding tests. As I remember, the 2011 PyCon sprint partly focused on adding tests. Were they not added to 3.2?

If the test is correct, it cannot break anything, and can catch future reversions (which do happen occasionally), besides helping other implementations. 3.2 will soon be a moot point, but I hope other implementations seriously work on 3.3 versions, and that we are willing to improve its test suite after the initial release.

If we are split here, discussion could move to pydev. It is not unusual that a specific issue raises a generic issue that needs broader discussion, but with the specific issue as a focal point.

Anyway, until there is a patch that we agree is technically correct, no decision need be made about commitment and the underlying generic policy.
msg160996 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-05-17 18:28
> It appears that os.stat did not check for embedded nulls but just passed the string to Windows.

JFTR, we have fixed it for open() because it's a potential security problem. I have implemented a helper function for checking for NUL chars: _PyUnicode_HasNULChars

See the already mentioned #13848.
msg221845 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-29 14:09
The type and versions fields have been set to what I think they ought to be, as I couldn't find any reference to this issue on python-dev.  Note that #13848 is closed as fixed and I've asked Victor if he can pick up #13617.
msg270954 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-07-21 19:20
Attached patch adds tests for common members of Lib/genericpath.py.
msg270965 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-07-21 22:44
issue13849.diff: LGTM.

If the tests pass on Python 2.7 and 3.5, I think that it's worth to add the tests to these versions as well.
msg271055 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-23 04:32
New changeset 899f06eb390c by Berker Peksag in branch '3.5':
Issue #13849: Add tests for null byte checking in test_genericpath
https://hg.python.org/cpython/rev/899f06eb390c

New changeset 9498736fbd8f by Berker Peksag in branch 'default':
Issue #13849: Merge from 3.5
https://hg.python.org/cpython/rev/9498736fbd8f
msg271056 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-07-23 04:35
Thanks for your review, Victor. I don't care about 2.7 at this point so I went with 3.5+ :)
msg271059 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-23 05:21
http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.5/builds/1072/steps/test/logs/stdio
======================================================================
FAIL: test_null_bytes (test.test_genericpath.TestGenericTest) (attr='getsize')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\buildbot.python.org\3.5.kloth-win64\build\lib\test\test_genericpath.py", line 285, in test_null_bytes
    self.assertEqual(str(cm.exception), 'embedded null byte')
AssertionError: 'stat: embedded null character' != 'embedded null byte'
- stat: embedded null character
+ embedded null byte
msg271061 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-23 05:43
New changeset 9e997c8f4876 by Berker Peksag in branch '3.5':
Issue #13849: Fix test_null_bytes under Windows
https://hg.python.org/cpython/rev/9e997c8f4876

New changeset a28abe83cf5c by Berker Peksag in branch 'default':
Issue #13849: Merge from 3.5
https://hg.python.org/cpython/rev/a28abe83cf5c
msg271075 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-23 13:20
Buildbots seem happier now
History
Date User Action Args
2016-07-23 13:20:44martin.pantersetstatus: open -> closed

messages: + msg271075
2016-07-23 05:43:09python-devsetmessages: + msg271061
2016-07-23 05:21:05martin.pantersetstatus: closed -> open
nosy: + martin.panter
messages: + msg271059

2016-07-23 04:35:17berker.peksagsetstatus: open -> closed
resolution: fixed
messages: + msg271056

stage: patch review -> resolved
2016-07-23 04:32:17python-devsetnosy: + python-dev
messages: + msg271055
2016-07-21 22:51:27BreamoreBoysetnosy: - BreamoreBoy
2016-07-21 22:44:45vstinnersetmessages: + msg270965
2016-07-21 19:20:27berker.peksagsetfiles: + issue13849.diff

versions: + Python 3.6, - Python 3.4
keywords: + patch
nosy: + berker.peksag

messages: + msg270954
stage: needs patch -> patch review
2014-06-29 14:09:52BreamoreBoysetversions: + Python 3.4, Python 3.5, - Python 2.7, Python 3.2, Python 3.3
nosy: + BreamoreBoy

messages: + msg221845

type: enhancement
2012-05-17 18:28:36hyneksetmessages: + msg160996
2012-05-17 16:45:35Arfreversetnosy: + Arfrever
2012-05-16 19:53:24terry.reedysetmessages: + msg160925
2012-05-16 17:20:15alexsetmessages: + msg160891
2012-05-16 13:45:23r.david.murraysetstage: needs patch
2012-05-16 13:44:06pitrousetmessages: + msg160863
2012-05-16 12:28:59hyneksetmessages: + msg160847
2012-02-15 21:13:13vstinnersetnosy: + vstinner
messages: + msg153436
2012-01-27 23:35:43pitrousetnosy: + pitrou

messages: + msg152141
versions: + Python 3.2, Python 3.3
2012-01-27 23:26:35terry.reedysetnosy: + terry.reedy
messages: + msg152140
2012-01-24 13:49:57loewissetmessages: + msg151908
2012-01-24 13:46:32loewissetmessages: + msg151907
2012-01-24 13:31:45hyneksetnosy: + hynek
2012-01-24 13:25:16r.david.murraysetmessages: + msg151904
2012-01-24 13:21:50r.david.murraysetmessages: + msg151903
2012-01-24 08:12:10loewissetmessages: + msg151883
2012-01-24 02:42:06r.david.murraysetnosy: + r.david.murray
messages: + msg151879
2012-01-24 02:23:36loewissetnosy: + loewis
messages: + msg151878
2012-01-24 02:05:53alexcreate