Opened 18 months ago
Closed 18 months ago
#28403 closed defect (fixed)
py3: crypto/block_cipher/present.py doctest failures
Reported by:  jhpalmieri  Owned by:  

Priority:  blocker  Milestone:  sage8.9 
Component:  python3  Keywords:  
Cc:  ghyhxnf  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  John Palmieri, Kwankyu Lee 
Report Upstream:  N/A  Work issues:  
Branch:  7c494ca (Commits, GitHub, GitLab)  Commit:  7c494caca41bc96e61c80bee133f2c324a0747a6 
Dependencies:  Stopgaps: 
Description
After #27573, there are two Python 3 doctest failures in present.py:
sage t warnlong 61.3 src/sage/crypto/block_cipher/present.py ********************************************************************** File "src/sage/crypto/block_cipher/present.py", line 42, in sage.crypto.block_cipher.present Failed example: cipher.sbox = SBox(range(16)) Exception raised: Traceback (most recent call last): File "/Users/palmieri/Desktop/Sage_stuff/sage_builds/PYTHON3/sage8.9.beta8/local/lib/python3.7/sitepackages/sage/doctest/forker.py", line 681, in _run self.compile_and_execute(example, compiler, test.globs) File "/Users/palmieri/Desktop/Sage_stuff/sage_builds/PYTHON3/sage8.9.beta8/local/lib/python3.7/sitepackages/sage/doctest/forker.py", line 1105, in compile_and_execute exec(compiled, globs) File "<doctest sage.crypto.block_cipher.present[9]>", line 1, in <module> cipher.sbox = SBox(range(Integer(16))) File "/Users/palmieri/Desktop/Sage_stuff/sage_builds/PYTHON3/sage8.9.beta8/local/lib/python3.7/sitepackages/sage/crypto/sbox.py", line 157, in __init__ raise TypeError("No lookup table provided.") TypeError: No lookup table provided. ********************************************************************** File "src/sage/crypto/block_cipher/present.py", line 44, in sage.crypto.block_cipher.present Failed example: cipher.encrypt(plaintext=0x1234, key=0x0).hex() Expected: '1234' Got: 'cccccccccccc56b9'
The problem is that in Python 3, range(...)
does not return a list as it does in Python 3, so the SBox.__init__
method doesn't handle it well.
Change History (18)
comment:1 Changed 18 months ago by
comment:2 Changed 18 months ago by
If you really want to test for iterables, maybe you should do
from collections.abc import Iterable if isinstance(args[0], Iterable): ...
(See https://stackoverflow.com/questions/1952464/inpythonhowdoidetermineifanobjectisiterable for more suggestions.)
comment:3 Changed 18 months ago by
 Cc ghyhxnf added
comment:4 Changed 18 months ago by
We should also (once fixed) add "crypto" to "src/ext/doctest/python3knownpassing.txt"
comment:5 Changed 18 months ago by
 Branch set to u/chapoton/28403
 Commit set to 41b4cacb580830cfd82ffd4ca7e1681955b1af3c
 Status changed from new to needs_review
comment:6 Changed 18 months ago by
 Reviewers set to John Palmieri
This looks good to me. If any crypto people have opinions, please let us know soon. Let's flip this to a positive review in a day or two if we don't hear anything.
comment:7 Changed 18 months ago by
 Status changed from needs_review to needs_work
Why not
 elif len(args) == 1 and isinstance(args[0], (list, tuple)): + elif len(args) == 1:
In other words: instead of checking whether it's a sequence, assume that it is. That's much more in spirit with duck typing. If it's not, we'll get an error when iterating over it.
comment:8 Changed 18 months ago by
 Commit changed from 41b4cacb580830cfd82ffd4ca7e1681955b1af3c to a766beb59c6c87045886f751001dff1194dd5b8a
Branch pushed to git repo; I updated commit sha1. New commits:
a766beb  use suggested ducktyping in sbox.py

comment:9 Changed 18 months ago by
 Status changed from needs_work to needs_review
Here it is, with belts and suspenders.
comment:10 Changed 18 months ago by
With that most recent change, the doctest doesn't need to be changed from range(16)
to list(range(16))
: range(16)
works.
comment:11 Changed 18 months ago by
Yes, that's the reason for me talking about beltandsuspenders.. Do you require me to undo this change, or can you live with it ?
comment:12 Changed 18 months ago by
I can live with it.
comment:13 Changed 18 months ago by
I prefer recovering the original doctest. list
now is just redundant.
comment:14 Changed 18 months ago by
 Commit changed from a766beb59c6c87045886f751001dff1194dd5b8a to 7c494caca41bc96e61c80bee133f2c324a0747a6
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7c494ca  trac 28403 fix crypto python3 regression

comment:15 Changed 18 months ago by
Ok, then. I bow to your common request. Please review.
comment:16 Changed 18 months ago by
 Reviewers changed from John Palmieri to John Palmieri, Kwankyu Lee
 Status changed from needs_review to positive_review
Thank you.
comment:17 Changed 18 months ago by
I agree with this latest version.
comment:18 Changed 18 months ago by
 Branch changed from u/chapoton/28403 to 7c494caca41bc96e61c80bee133f2c324a0747a6
 Resolution set to fixed
 Status changed from positive_review to closed
I can think of two solutions: either change the doctest to use
SBox(list(range(16))
, or this change tocrypto/sbox.py
:src/sage/crypto/sbox.py
)):The first way is safer. The second way may be a little closer to the documentation: the documentation says that
SBox.__init__
takes as input "a finite iterable". The source code checks for lists and tuples, but not for other iterables likerange
. I don't know anything about the crypto code, though, so I am not the right person to judge the right way to fix this.