cremona_letter_code does not handle erroneous input gracefully.

[sagetrac-drkirkby]

Although I'm not a mathematician, I believe from reading the help page and looking at the examples, the Sage command cremona_letter_code should probably only work for non-negative integers. However, this is not the case.

sage: cremona_letter_code(23.23) # Invalid input, gives output.
'x'
sage: cremona_letter_code(-3)   # Invalid input, hangs using lots of CPU time.

Any attempt to send erroneous input to a command should generate an error message.

There seems to be three problems with this.

• Negative integers hang Sage, using lots of CPU time.
• The documentation says "integers" but I think it should say "non-negative integers" - at least all the examples are either 0 or positive integers.
• Floating point numbers produce output, despite the fact the documentation says the argument should be an integer.

This was found by using Fuzz Testing
http://en.wikipedia.org/wiki/Fuzz_testing
where one purposely supplies invalid input, to check how software behaves.

http://www.ibm.com/developerworks/java/library/j-fuzztest.html

says Fuzz testing is a simple technique that can have a profound effect on your code quality.

I've marked it as a minor priority, as clearly its not a huge bug. But it would be worth fixing it.

Dave

Component: elliptic curves

Keywords: databases

Author: John Cremona

Reviewer: David Kirkby, Jeroen Demeyer

Merged: sage-4.6.1.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/10105

[JohnCremona]

comment:2

You are quite right: any input not a non-negative integer is bad, and the code should check this.

It's also true that this function need not be in the global namespace, it's a technical helper function. so in sage/databases/all.py:

from cremona import CremonaDatabase, \
     cremona_letter_code, parse_cremona_label, \
     old_cremona_letter_code, is_optimal_id

only the first import should be there. For another ticket perhaps, since certainly removing those global imports will result in lots of local ones being needed.

[sagetrac-drkirkby]

comment:3

Replying to @JohnCremona:

You are quite right: any input not a non-negative integer is bad, and the code should check this.

I thought I probably was, though since I'm not a mathematician, I don't understand much of this.

I just wrote a little script which stuck some data into every function and looked to see for any that crashed or hung - I've no idea if the output of the others is correct or not.

It's also true that this function need not be in the global namespace, it's a technical helper function. so in sage/databases/all.py:

from cremona import CremonaDatabase, \
     cremona_letter_code, parse_cremona_label, \
     old_cremona_letter_code, is_optimal_id

only the first import should be there. For another ticket perhaps, since certainly removing those global imports will result in lots of local ones being needed.

Yes, those other changes do sound like they need to be on another ticket. I'll leave you to create that one.

I would have thought this one pretty easy to fix, though I'm not going to attempt it myself as I'm not a competent Python programmer. You are the best person to do it.

Dave

[JohnCremona]

Changed keywords from none to databases

[JohnCremona]

Author: John Cremona

[JohnCremona]

comment:4

Attachment: trac_10105-cremona-letter-code.patch.gz

Here's a patch + tests.

[sagetrac-drkirkby]

comment:5

I'll test the patch.

A minor point, but there is a grammatical error - the word are is missing.

Cremona letter codes are only defined for non-negative integers

Dave

[sagetrac-drkirkby]

comment:6

I've not downloaded the latest alpha3 yet, and since I'm using my alpha2 for testing, I will need some time to download the alpha3 and will test with that.

Dave

[JohnCremona]

comment:7

Replying to @sagetrac-drkirkby:

I'll test the patch.

A minor point, but there is a grammatical error - the word are is missing.

Cremona letter codes are only defined for non-negative integers

Dave

That was an abbreviated message rather than a mistake, but I have corrected it now.
(Replacement patch in a few seconds.)
And I too am still building alpha3 so this was made for alpha2, though I think it highly unlikely that will change matters.

By the way, the only places where this function is used outside its own file are in modular/abvar/abvar and .../abvar_newform, where it is explicitly imported.

[JohnCremona]

updated for grammar correction

[sagetrac-drkirkby]

comment:8

Attachment: trac_10105-cremona-letter-code.2.patch.gz

John, it does not check if the input is an integer or not - it gives an output for a floating point number too. I would hope it would generate an error message for that too.

sage: cremona_letter_code(3.14192543)
'd'

[JohnCremona]

comment:9

Hmmm. Is there a better way to test this than

if type(n) in [type(int(1)), type(Integer(1))]:

?

[JohnCremona]

replaces all previous

[JohnCremona]

comment:10

Attachment: trac_10105-cremona-letter-code.3.patch.gz

Third attempt!

[sagetrac-drkirkby]

Reviewer: David Kirkby

[sagetrac-drkirkby]

comment:11

Thanks John. I think its third time lucky, as I can't find any faults with this.

I've tried every combination of junk I can think of to throw at cremona_letter_code(), but it always behaves properly. It will not accept any augments except one non-negative integer.

It passes the doctest

drkirkby@hawk:~/sage-4.6.alpha3$ ./sage -t  -long devel/sage/sage/databases/cremona.py
sage -t -long "devel/sage/sage/databases/cremona.py"        
	 [3.9 s]
 
----------------------------------------------------------------------
All tests passed!
Total time for all tests: 3.9 seconds

However, I don't feel my Python skills are sufficient to press the positive review, so I hope someone with more Python knowledge will look at this. But I can't fault it myself.

[jdemeyer]

comment:12

I'm having a look.

[jdemeyer]

Changed reviewer from David Kirkby to David Kirkby, Jeroen Demeyer

[jdemeyer]

Attachment: trac_10105-cremona-letter-code.4.patch.gz

Removed "pass", replaces previous patches

[jdemeyer]

comment:13

I removed the unneeded "pass" statement. Apart from that, everything look fine.

[JohnCremona]

comment:14

Replying to @jdemeyer:

I removed the unneeded "pass" statement. Apart from that, everything look fine.

Thanks -- I rearranged the code several times and that "pass" got left in my mistake.

[jdemeyer]

Merged: sage-4.6.1.alpha0