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: "int" comment in marshal.c is outdated
Type: Stage: resolved
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: georg.brandl, larry, loewis, pitrou, python-dev, vstinner
Priority: normal Keywords: patch

Created on 2013-10-14 12:44 by larry, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
larry.marshal.outdated.comment.r1.diff larry, 2013-10-14 12:44 review
Messages (6)
msg199883 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-10-14 12:44
In r_string() (read a string) in marshal.c we see this comment:

    /* The result fits into int because it must be <=n. */
    read = fread(p->buf, 1, n, p->fp);

This comment was first committed in r36501 by MvL.  Back then the "read" and "n" variables were int, but (of course) the return size of fread was size_t.  Since then, both n and read have become ssize_t.

I suggest changing the wording slightly anyway, because I had to meditate on what the comment was originally trying to say.  I suggest:

    /* The result fits into ssize_t because n is ssize_t. */

Patch appended too.  Bikeshedding away!

Should this be fixed in previous versions too?
msg199899 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2013-10-14 14:47
<aside> The "rXXX" notation is reserved for SVN revision numbers, which are unambiguous.  The revision numbers in Mercurial are specific to each clone. </aside>

Other than that, LGTM.
msg199905 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-14 15:09
Well, the comment should actually be removed, it's just pointless.
msg199907 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2013-10-14 15:17
I agree that the comment can be removed. If the code is (now) statically type-safe, there is no point in keeping it.
msg199939 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-10-14 18:51
New changeset 1309fee48908 by Antoine Pitrou in branch 'default':
Close #19260: remove outdated comment in marshal.c
http://hg.python.org/cpython/rev/1309fee48908
msg199972 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-10-15 01:22
Martin: The code is not statically type-safe, but it is mechanically safe, which I think is why you wrote the comment in the first place.

The return value of fread() is size_t, but the "read" variable is Py_ssize_t.  So the function *could* theoretically return a value that would overflow the variable.  However, this can't happen in practice because the input size ("n") is also Py_ssize_t.  And fread() will never return a number larger than the number of bytes requested.  QED the function will never return a number larger than one that could be stored in Py_ssize_t.

If the comment originally had merit, then it still has merit.  It was referring to this exact same situation, except back then the "n" and "read" variables were both type "int".  They were both changed to "Py_ssize_t" some time ago but the comment was not updated.

Since Martin contributed the original code and comment I leave it to him to decide its fate, Antoine's shooting-from-the-hip aside.
History
Date User Action Args
2022-04-11 14:57:52adminsetgithub: 63459
2013-10-15 01:22:00larrysetmessages: + msg199972
2013-10-14 18:51:01python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg199939

resolution: fixed
stage: resolved
2013-10-14 15:21:47vstinnersetnosy: + vstinner
2013-10-14 15:17:01loewissetnosy: + loewis
messages: + msg199907
2013-10-14 15:09:11pitrousetnosy: + pitrou
messages: + msg199905
2013-10-14 14:47:23georg.brandlsetnosy: + georg.brandl
messages: + msg199899
2013-10-14 12:44:36larrycreate