Re: [Maria-developers] fbe1dad15a1: Ensure that we do not allocate strings bigger than 4G in String objects.
Hi, Monty! On Apr 16, Michael Widenius wrote:
revision-id: fbe1dad15a1 (mariadb-10.5.2-555-gfbe1dad15a1) parent(s): 57c19902326 author: Michael Widenius <michael.widenius@gmail.com> committer: Michael Widenius <michael.widenius@gmail.com> timestamp: 2021-03-25 12:06:34 +0200 message:
Ensure that we do not allocate strings bigger than 4G in String objects.
This is needed as we are using uint32 for allocated and current length.
diff --git a/sql/sql_string.cc b/sql/sql_string.cc index 9c57bb22085..fedf5f4a48a 100644 --- a/sql/sql_string.cc +++ b/sql/sql_string.cc @@ -37,6 +37,8 @@ bool Binary_string::real_alloc(size_t length) DBUG_ASSERT(arg_length > length); if (arg_length <= length) return TRUE; /* Overflow */ + if (arg_length >= UINT_MAX32) + return FALSE; str_length=0; if (Alloced_length < arg_length) { @@ -45,7 +47,6 @@ bool Binary_string::real_alloc(size_t length) arg_length,MYF(MY_WME | (thread_specific ? MY_THREAD_SPECIFIC : 0))))) return TRUE; - DBUG_ASSERT(length < UINT_MAX32); Alloced_length=(uint32) arg_length; alloced=1;
I liked assert more. It meant that we should never under any circumstances request more than 4G from Binary_string::real_alloc. You're saying that it's ok to do it and Binary_string::real_alloc should be ready to deal with it. I'd say the first approach was better, >4G should just never happen. Regards, Sergei
Hi! On Fri, Apr 16, 2021 at 8:08 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Monty!
On Apr 16, Michael Widenius wrote:
revision-id: fbe1dad15a1 (mariadb-10.5.2-555-gfbe1dad15a1) parent(s): 57c19902326 author: Michael Widenius <michael.widenius@gmail.com> committer: Michael Widenius <michael.widenius@gmail.com> timestamp: 2021-03-25 12:06:34 +0200 message:
Ensure that we do not allocate strings bigger than 4G in String objects.
This is needed as we are using uint32 for allocated and current length.
diff --git a/sql/sql_string.cc b/sql/sql_string.cc index 9c57bb22085..fedf5f4a48a 100644 --- a/sql/sql_string.cc +++ b/sql/sql_string.cc @@ -37,6 +37,8 @@ bool Binary_string::real_alloc(size_t length) DBUG_ASSERT(arg_length > length); if (arg_length <= length) return TRUE; /* Overflow */ + if (arg_length >= UINT_MAX32) + return FALSE; str_length=0; if (Alloced_length < arg_length) { @@ -45,7 +47,6 @@ bool Binary_string::real_alloc(size_t length) arg_length,MYF(MY_WME | (thread_specific ? MY_THREAD_SPECIFIC : 0))))) return TRUE; - DBUG_ASSERT(length < UINT_MAX32); Alloced_length=(uint32) arg_length; alloced=1;
I liked assert more. It meant that we should never under any circumstances request more than 4G from Binary_string::real_alloc.
I did not just remove the assert for the fun of it. The problem with the assert is there was ways with wrong test cases to get sql_string to allocate too big strings. Instead of having these fail with an assert, we now get a signal 'allocation failed' for these (for code that checks the string argument). I must have got this problem while testing something. It may however be a good idea to generate a malloc fail error for this case. Regards. Monty
participants (2)
-
Michael Widenius -
Sergei Golubchik