Opened 8 years ago

Closed 21 months ago

#27397 closed Bug (fixed)

Querying with an integer larger than SQLite supports crashes with OverflowError

Reported by: Ramin Farajpour Cami Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 1.10
Severity: Normal Keywords: SQLite, Error Handling
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi,

i don't know, you accept this behavior Overflow on SQLite or there is max length here?

Step :

http://127.0.0.1:8000/admin/auth/user/1111111111111111111111/change/

Error :

OverflowError at /admin/auth/user/1111111111111111111111/change/
Python int too large to convert to SQLite INTEGER
Request Method:	GET
Request URL:	http://127.0.0.1:8000/admin/auth/user/1111111111111111111111/change/
Django Version:	1.10.2
Exception Type:	OverflowError
Exception Value:	
Python int too large to convert to SQLite INTEGER
Exception Location:	C:\Python27\lib\site-packages\django\db\backends\sqlite3\operations.py in _quote_params_for_last_executed_query, line 129
Python Executable:	C:\Python27\python.exe
Python Version:	2.7.11
Python Path:	
['E:/Programmer Language/NodeJS/untitled1',
 'C:\\Users\\RaminFP\\.IntelliJIdea14\\config\\plugins\\python\\helpers\\pydev',
 'C:\\Python27\\lib\\site-packages\\six-1.10.0-py2.7.egg',
 'C:\\Python27\\lib\\site-packages\\pisa-3.0.33-py2.7.egg',
 'C:\\Python27\\lib\\site-packages\\sqlacodegen-1.1.6-py2.7.egg',
 'C:\\Python27\\lib\\site-packages\\inflect-0.2.5-py2.7.egg',
 'C:\\Python27\\lib\\site-packages\\0x10c_asm-0.0.2-py2.7.egg',
 'C:\\Python27\\lib\\site-packages\\jpype1-0.6.1-py2.7-win32.egg',
 'C:\\Python27\\lib\\site-packages\\simpleaes-1.0-py2.7.egg',
 'C:\\Python27\\lib\\site-packages\\celery-3.1.23-py2.7.egg',
 'C:\\Python27\\lib\\site-packages\\kombu-3.0.35-py2.7.egg',
 'C:\\Python27\\lib\\site-packages\\anyjson-0.3.3-py2.7.egg',
 'C:\\Python27\\lib\\site-packages\\pythonnet-2.1.0.dev1-py2.7-win32.egg',
 'E:\\Programmer Language\\NodeJS\\untitled1',
 'C:\\Windows\\SYSTEM32\\python27.zip',
 'C:\\Python27\\DLLs',
 'C:\\Python27\\lib',
 'C:\\Python27\\lib\\plat-win',
 'C:\\Python27\\lib\\lib-tk',
 'C:\\Python27',
 'C:\\Python27\\lib\\site-packages',
 'c:\\python27\\lib\\site-packages']
Server time:	Fri, 28 Oct 2016 11:43:05 +0000

Attachments (2)

excep.diff (833 bytes ) - added by Ramin Farajpour Cami 8 years ago.
Overflow_handel.diff (1.9 KB ) - added by Ramin Farajpour Cami 8 years ago.

Download all attachments as: .zip

Change History (37)

comment:1 by nickparkin, 8 years ago

Owner: changed from nobody to nickparkin
Status: newassigned

comment:2 by Tim Graham, 8 years ago

Summary: OverflowError at /admin/auth/user/<.id>/changeQuerying with an integer larger than SQLite supports crashes with OverflowError
Triage Stage: UnreviewedAccepted

I'm not sure how to fix that, but it would be nice not to crash if possible.

comment:3 by nickparkin, 8 years ago

Summary: Querying with an integer larger than SQLite supports crashes with OverflowErrorOverflowError at /admin/auth/user/<.id>/change
Triage Stage: AcceptedUnreviewed

I have reproduced this error. This is an error on SQLite side as the integer is too large to be handled by SQLite.

You will see this error as above when you search for User ID 1111111111111111111111

The max integer is 9223372036854775807. Using this will give you the following error:

Page not found (404)
Request Method: GET
Request URL: http://127.0.0.1:8000/admin/auth/user/9223372036854775807/change/
Raised by: django.contrib.admin.options.change_view

Out of interest, if you use the user ID 9223372036854775808, you will get the same error you saw.

Was not able to recreate the server crashing on my side. It held up fine and simply throws a Server Error when running under production (Debug = False).

comment:6 by nickparkin, 8 years ago

in reply to:  4 comment:7 by Ramin Farajpour Cami, 8 years ago

Replying to nickparkin:

Looking into if this happens on Postgres now to see if Django is able to handle of these errors.

query can not check max length of valid integer? if check raise on excpetion

Page not found (404)
Request Method: GET
Request URL: ​http://127.0.0.1:8000/admin/auth/user/9223372036854775807/change/
Raised by: django.contrib.admin.options.change_view

check range int after execute query on SQLLite ,

comment:8 by nickparkin, 8 years ago

Have confirmed this error by Django handled in Postgres: e.g. http://127.0.0.1:8000/admin/auth/user/922337203685477582000000000000000000/change/

As such, keeping this open as this could be handled for SQLLite.

Version 0, edited 8 years ago by nickparkin (next)

comment:9 by nickparkin, 8 years ago

Keywords: SQLite Error Handling added

comment:10 by Ramin Farajpour Cami, 8 years ago

Summary: OverflowError at /admin/auth/user/<.id>/changeQuerying with an integer larger than SQLite supports crashes with OverflowError
Triage Stage: UnreviewedAccepted

i test on mysql with many id integer nothing of overflow

comment:11 by Ramin Farajpour Cami, 8 years ago

Owner: changed from nickparkin to Ramin Farajpour Cami

by Ramin Farajpour Cami, 8 years ago

Attachment: excep.diff added

comment:12 by Ramin Farajpour Cami, 8 years ago

Hi Tim,

please look this patch and your feedback ,

comment:13 by Ramin Farajpour Cami, 8 years ago

comment:14 by Tim Graham, 8 years ago

Assuming a 404 should be raised wherever an OverflowError is raised doesn't seem correct. A fix at the query level seems more correct. Tests are also needed.

comment:15 by Ramin Farajpour Cami, 8 years ago

Which query change? (please give me python file this query)

max length range define on sqllite wherever return OverflowError ,

comment:16 by Tim Graham, 8 years ago

For example, I was thinking that QuerySet.filter() with a filter parameter value that overflows (and hence can't match anything) might return zero results.

comment:17 by Ramin Farajpour Cami, 8 years ago

i think you means this , this patch is a very easy Tim, and best way,

$ git diff HEAD
diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py
index 499d27d..58049ed 100644
--- a/django/contrib/admin/options.py
+++ b/django/contrib/admin/options.py
@@ -668,7 +668,7 @@ class ModelAdmin(BaseModelAdmin):
         try:
             object_id = field.to_python(object_id)
             return queryset.get(**{field.name: object_id})
-        except (model.DoesNotExist, ValidationError, ValueError):
+        except (model.DoesNotExist, ValidationError, ValueError, OverflowError):
             return None

     def get_changelist_form(self, request, **kwargs):

view :

Page not found (404)
Request Method:	GET
Request URL:	http://127.0.0.1:8000/admin/auth/user/1111111111111111111111111111/change/
Raised by:	django.contrib.admin.options.change_view
user object with primary key u'1111111111111111111111111111' does not exist.

Overflow Code ;

return queryset.get(**{field.name: object_id})

comment:18 by Ramin Farajpour Cami, 8 years ago

i'm missing for use which test folders tests ?

please give me sample python file tests object_id, ready for new PR ,

Thanks,
Ramin

comment:19 by Tim Graham, 8 years ago

The issue affects all queries, not just those from the admin. A proper fix my go somewhere in the SQLite database backend (django/db/backends/sqlite3) but I'm not sure.

comment:20 by Ramin Farajpour Cami, 8 years ago

are you sure "The issue affects all queries, not just those from the admin"? where is issue? i think just happen object_id change_view _ _

i test patch is working (http://127.0.0.1:8000/admin/auth/user/1111111111111111111111/change/),
i need more information a fix,what your means "A proper fix my go somewhere in the SQLite database backend (django/db/backends/sqlite3)"

comment:21 by Ramin Farajpour Cami, 8 years ago

still i'm not sure because i test http://127.0.0.1:8000/admin/auth/group/11111111111111111111111111111111111111111/change/ nothing error of OverflowError,

comment:23 by Tim Graham, 8 years ago

The idea is to fix things so that Question.objects.filter(id=10000000000000000000) doesn't crash. Doing so should also fix the admin.

comment:24 by Ramin Farajpour Cami, 8 years ago

>CREATE TABLE numbers (i INTEGER, r REAL, t TEXT, b BLOB);

>INSERT INTO numbers VALUES (9223372036854775807, 9223372036854775807, 9223372036854775807, 9223372036854775807); 

> SELECT * FROM numbers;
i                     r                     t                     b
--------------------  --------------------  --------------------  --------------------
9223372036854775807   9.22337203685478e+18  9223372036854775807   9223372036854775807

> SELECT typeof(i),typeof(r),typeof(t),typeof(b) FROM numbers;
typeof(i)   typeof(r)   typeof(t)   typeof(b)
----------  ----------  ----------  ----------
integer     real        text        integer

> SELECT i+1,r+1,t+1,b+1 FROM numbers;
i+1                   r+1                   t+1                   b+1
--------------------  --------------------  --------------------  --------------------
-9223372036854775808  9.22337203685478e+18  -9223372036854775808  -9223372036854775808

you see shift +1 and -9223372036854775808

INTEGER. The value is a signed integer, stored in 1, 2, 3, 4, 6, or 8 bytes depending on the magnitude of the value.

REAL. The value is a floating point value, stored as an 8-byte IEEE floating point number.

That is, you can only store values from 2**63 to (2**63-1)

Be careful when you need to store large numbers in SQLite. If you really need to support unsigned 64-bit numbers, ?? but it makes a SQLite bad choice for developing

please look :
https://github.com/python/cpython/blob/master/Modules/_sqlite/util.c#L129&L153

Last edited 8 years ago by Ramin Farajpour Cami (previous) (diff)

comment:25 by Ramin Farajpour Cami, 8 years ago

Tim, i see https://docs.djangoproject.com/en/dev/ref/models/fields/#bigintegerfield ,

do you think change fieldid int to biginteger or BigAutoField is good?

Last edited 8 years ago by Ramin Farajpour Cami (previous) (diff)

comment:26 by Ramin Farajpour Cami, 8 years ago

final patch

diff --git a/django/db/backends/sqlite3/base.py b/django/db/backends/sqlite3/base.py
index 66ad278..36c1351 100644
--- a/django/db/backends/sqlite3/base.py
+++ b/django/db/backends/sqlite3/base.py
@@ -325,8 +325,11 @@ class SQLiteCursorWrapper(Database.Cursor):
         if params is None:
             return Database.Cursor.execute(self, query)
         query = self.convert_query(query)
-        return Database.Cursor.execute(self, query, params)
-
+        try:
+            return Database.Cursor.execute(self, query, params)
+        except OverflowError:
+            return None
+            
     def executemany(self, query, param_list):
         query = self.convert_query(query)
         return Database.Cursor.executemany(self, query, param_list)
diff --git a/django/db/backends/sqlite3/operations.py b/django/db/backends/sqlite3/operations.py
index 47a26b5..6404ea6 100644
--- a/django/db/backends/sqlite3/operations.py
+++ b/django/db/backends/sqlite3/operations.py
@@ -122,6 +122,8 @@ class DatabaseOperations(BaseDatabaseOperations):
         # Native sqlite3 cursors cannot be used as context managers.
         try:
             return cursor.execute(sql, params).fetchone()
+        except OverflowError:
+            return None    
         finally:
             cursor.close()

usage :

>>> User.objects.get(id=1111111111111111111111111111111111111111111111111111111111111111111111111111111111)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "C:\Python27\lib\site-packages\django\db\models\manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "C:\Python27\lib\site-packages\django\db\models\query.py", line 385, in get
    self.model._meta.object_name
DoesNotExist: User matching query does not exist.
>>> User.objects.get(id=111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "C:\Python27\lib\site-packages\django\db\models\manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "C:\Python27\lib\site-packages\django\db\models\query.py", line 385, in get
    self.model._meta.object_name
DoesNotExist: User matching query does not exist.
>>> User.objects.filter(id=1111111111111111111111111111111111111111111111111111111111111111111111111111111111)
<QuerySet []>
>>>
Last edited 8 years ago by Ramin Farajpour Cami (previous) (diff)

comment:27 by Ramin Farajpour Cami, 8 years ago

Has patch: set

by Ramin Farajpour Cami, 8 years ago

Attachment: Overflow_handel.diff added

comment:28 by Ramin Farajpour Cami, 8 years ago

this change is not important i will remove code,

diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py
index 499d27d..58049ed 100644
--- a/django/contrib/admin/options.py
+++ b/django/contrib/admin/options.py
@@ -668,7 +668,7 @@ class ModelAdmin(BaseModelAdmin):
         try:
             object_id = field.to_python(object_id)
             return queryset.get(**{field.name: object_id})
-        except (model.DoesNotExist, ValidationError, ValueError):
+        except (model.DoesNotExist, ValidationError, ValueError, OverflowError):
             return None

     def get_changelist_form(self, request, **kwargs):

do you confirm fix and patch for PR?

https://github.com/django/django/blob/master/tests/queries/modles.py

class Ticket27397(models.Model):
    id = models.AutoField(primary_key=True)
    name = models.CharField(max_length=10)

https://github.com/django/django/blob/master/tests/queries/tests.py

    def test_ticket27397(self):
        self.assertEqual(len(Ticket27397.objects.filter(id=111111111111111111111111111111111111)),0)

Last edited 8 years ago by Ramin Farajpour Cami (previous) (diff)

comment:29 by Ramin Farajpour Cami, 8 years ago

Needs tests: set

comment:30 by Tim Graham, 8 years ago

I'm not sure if the fix is the cleanest one, but I'd have to look at the issue in more detail. I guess you can create a pull request at this point. You should be able to reuse an existing model for the test.

comment:31 by Tim Graham, 8 years ago

Needs tests: unset
Patch needs improvement: set

As noted on the PR, the proposal doesn't work correctly for QuerySet.exclude().

comment:32 by Mariusz Felisiak, 4 years ago

Owner: Ramin Farajpour Cami removed
Status: assignednew

comment:33 by Simon Charette, 21 months ago

Owner: set to Simon Charette
Status: newassigned

comment:34 by Simon Charette, 21 months ago

Patch needs improvement: unset

Submitted a patch that will effectively only address the SQLite issue reported here when a patch for #34370 lands.

comment:35 by Mariusz Felisiak, 21 months ago

Patch needs improvement: set

comment:36 by Simon Charette, 21 months ago

Patch needs improvement: unset

comment:37 by Mariusz Felisiak, 21 months ago

Triage Stage: AcceptedReady for checkin

comment:38 by Mariusz Felisiak <felisiak.mariusz@…>, 21 months ago

Resolution: fixed
Status: assignedclosed

In dde2537:

Fixed #27397 -- Prevented integer overflows on integer field lookups.

This prevents a sqlite3 crash and address a potential DDoS vector on
PostgreSQL caused by full-table-scans on overflows.

Note: See TracTickets for help on using tickets.
Back to Top