Opened 4 months ago

Closed 4 months ago

#28403 closed defect (fixed)

py3: crypto/block_cipher/present.py doctest failures

Reported by: jhpalmieri Owned by:
Priority: blocker Milestone: sage-8.9
Component: python3 Keywords:
Cc: gh-yhxnf Merged in:
Authors: Frédéric Chapoton Reviewers: John Palmieri, Kwankyu Lee
Report Upstream: N/A Work issues:
Branch: 7c494ca (Commits) Commit: 7c494caca41bc96e61c80bee133f2c324a0747a6
Dependencies: Stopgaps:

Description

After #27573, there are two Python 3 doctest failures in present.py:

sage -t --warn-long 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/sage-8.9.beta8/local/lib/python3.7/site-packages/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/sage-8.9.beta8/local/lib/python3.7/site-packages/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/sage-8.9.beta8/local/lib/python3.7/site-packages/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 4 months ago by jhpalmieri

I can think of two solutions: either change the doctest to use SBox(list(range(16)), or this change to crypto/sbox.py:

  • src/sage/crypto/sbox.py

    diff --git a/src/sage/crypto/sbox.py b/src/sage/crypto/sbox.py
    index 200bd5ad55..5aa9e7257d 100644
    a b class SBox(SageObject): 
    149150            if R.characteristic() != 2:
    150151                raise TypeError("Only polynomials over rings with characteristic 2 allowed")
    151152            S = [poly(v) for v in sorted(R)]
    152         elif len(args) == 1 and isinstance(args[0], (list, tuple)):
     153        elif len(args) == 1 and isinstance(args[0], (list, tuple, range)):
    153154            S = args[0]
    154155        elif len(args) > 1:
    155156            S = args

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 like range. 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.

comment:2 Changed 4 months ago by jhpalmieri

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/in-python-how-do-i-determine-if-an-object-is-iterable for more suggestions.)

comment:3 Changed 4 months ago by klee

  • Cc gh-yhxnf added

comment:4 Changed 4 months ago by chapoton

We should also (once fixed) add "crypto" to "src/ext/doctest/python3-known-passing.txt"

comment:5 Changed 4 months ago by chapoton

  • Authors set to Frédéric Chapoton
  • Branch set to u/chapoton/28403
  • Commit set to 41b4cacb580830cfd82ffd4ca7e1681955b1af3c
  • Status changed from new to needs_review

please review


New commits:

41b4cactrac 28403 fix crypto python3 regression

comment:6 Changed 4 months ago by jhpalmieri

  • 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 4 months ago by jdemeyer

  • 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 4 months ago by git

  • Commit changed from 41b4cacb580830cfd82ffd4ca7e1681955b1af3c to a766beb59c6c87045886f751001dff1194dd5b8a

Branch pushed to git repo; I updated commit sha1. New commits:

a766bebuse suggested duck-typing in sbox.py

comment:9 Changed 4 months ago by chapoton

  • Status changed from needs_work to needs_review

Here it is, with belts and suspenders.

comment:10 Changed 4 months ago by jhpalmieri

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 4 months ago by chapoton

Yes, that's the reason for me talking about belt-and-suspenders.. Do you require me to undo this change, or can you live with it ?

comment:12 Changed 4 months ago by jhpalmieri

I can live with it.

comment:13 Changed 4 months ago by klee

I prefer recovering the original doctest. list now is just redundant.

comment:14 Changed 4 months ago by git

  • Commit changed from a766beb59c6c87045886f751001dff1194dd5b8a to 7c494caca41bc96e61c80bee133f2c324a0747a6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7c494catrac 28403 fix crypto python3 regression

comment:15 Changed 4 months ago by chapoton

Ok, then. I bow to your common request. Please review.

comment:16 Changed 4 months ago by klee

  • Reviewers changed from John Palmieri to John Palmieri, Kwankyu Lee
  • Status changed from needs_review to positive_review

Thank you.

comment:17 Changed 4 months ago by jdemeyer

I agree with this latest version.

comment:18 Changed 4 months ago by vbraun

  • Branch changed from u/chapoton/28403 to 7c494caca41bc96e61c80bee133f2c324a0747a6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.