Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable int to string conversion limit #941

Closed
wants to merge 1 commit into from

Conversation

kejcao
Copy link
Contributor

@kejcao kejcao commented Nov 25, 2023

Factorial[5000] gives the crash:

...
  File "/home/kjc/mathics-core/mathics/core/rules.py", line 269, in do_replace
    return self.function(evaluation=evaluation, **vars_noctx)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/kjc/mathics-core/mathics/builtin/makeboxes.py", line 360, in eval_general
    return expr.atom_to_boxes(f, evaluation)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/kjc/mathics-core/mathics/core/atoms.py", line 239, in atom_to_boxes
    return self.make_boxes(f.get_name())
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/kjc/mathics-core/mathics/core/atoms.py", line 249, in make_boxes
    return String(str(self._value))
                  ^^^^^^^^^^^^^^^^
ValueError: Exceeds the limit (7000 digits) for integer string conversion; use sys.set_int_max_str_digits() to increase the limit

The limit was introduced in Python 3.11 to prevent DoS attacks. That problem doesn't apply in Mathics case. This PR removes the limit.

@mmatera
Copy link
Contributor

mmatera commented Nov 25, 2023

Maybe it is worth controlling this with an entry in settings.

@rocky
Copy link
Member

rocky commented Nov 25, 2023

I too have some hesitation here.

The limit was introduced in Python 3.11 to prevent DoS attacks.

That may be how it got introduced, but I don't think it is the only reason it is there.

As best as I understand, Python conversion from decimal to integer takes quadratic time, and since this can cause a serious slowdown that it is a vector for DoS attack, it is something developers and Mathics3 users may need to be aware of.

Somewhere in the discussion, it is mentioned that there is a common idiom in network code that makes it easy for attackers to take advantage of this. But this kind of thing might be equally valid, if not very rarely used, here as well. We don't want CVE's registered against Mathics3 if we can avoid that.

To me, the thing that is most wrong is how we handle what happens - the code exits with that message.

If feels to me that it would be better to trap on this ValueError exception and check if this a integer-to-string conversion problem. If so, then we would turn this into a proper Mathics3 exception. And we could offer a Mathics3 -specific message and remedy, whatever we decide.

https://docs.python.org/3.11/library/stdtypes.html#int-max-str-digits suggests other ways this maximum value can be set : via a shell environment variable or via an -X option to the Python interpreter. I have tried these but haven't been able to see this get set.

(I have a vague recollection that we came across the issue where string to int conversion was taking a long time. I think we somehow worked around that by not requiring the conversion)

@mmatera
Copy link
Contributor

mmatera commented Dec 15, 2023

The issue come up in the formatting step:

In[1] :=  a=Factorial[5000];
Out[1] = None

is OK, but

In[2] :=  a

raises the error. I think the right way to handle this would be by adding an exception and implementing a system variable that fixes the maximum size.

@mmatera
Copy link
Contributor

mmatera commented Jan 16, 2024

@kejcao I think #949 covers this. If you think this is the case, please close the PR.

@kejcao kejcao closed this Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants