Re: [Maria-developers] 39e20ca7e28: MDEV-29748 ASAN errors or server crash in File_parser::parse upon concurrent view operations
Hi, Oleksandr, Looks good. Just one question below: On Oct 19, Oleksandr Byelkin wrote:
revision-id: 39e20ca7e28 (mariadb-10.3.36-107-g39e20ca7e28) parent(s): d6707ab11f6 author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2022-10-18 16:38:09 +0200 message:
MDEV-29748 ASAN errors or server crash in File_parser::parse upon concurrent view operations
Read the version of the view share when we read definition to prevent simultaniouse access to a view table SHARE (and so its MEM_ROOT) from different threads.
diff --git a/sql/sql_view.cc b/sql/sql_view.cc index 68a8410f559..1e4a740f871 100644 --- a/sql/sql_view.cc +++ b/sql/sql_view.cc @@ -1175,19 +1175,24 @@ static int mysql_register_view(THD *thd, TABLE_LIST *view, bool mariadb_view_version_get(TABLE_SHARE *share) { DBUG_ASSERT(share->is_view); + DBUG_ASSERT(share->tabledef_version.length == 0);
if (!(share->tabledef_version.str= (uchar*) alloc_root(&share->mem_root, MICROSECOND_TIMESTAMP_BUFFER_SIZE))) return TRUE; - share->tabledef_version.length= 0; // safety if the drfinition file is brocken
DBUG_ASSERT(share->view_def != NULL); if (share->view_def->parse((uchar *) &share->tabledef_version, NULL, view_timestamp_parameters, 1, &file_parser_dummy_hook)) + { + // safety if the definition file is brocken + share->tabledef_version.length= 0;
when can view_def->parse() fail? _Only_ if the definition is invalid? Can it succeed for one TABLE and fail for another, both of the same TABLE_SHARE?
return TRUE; + } DBUG_ASSERT(share->tabledef_version.length == MICROSECOND_TIMESTAMP_BUFFER_SIZE-1); + return FALSE; }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Sergei! On Wed, Oct 19, 2022 at 5:09 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Oleksandr,
Looks good. Just one question below:
On Oct 19, Oleksandr Byelkin wrote:
revision-id: 39e20ca7e28 (mariadb-10.3.36-107-g39e20ca7e28) parent(s): d6707ab11f6 author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2022-10-18 16:38:09 +0200 message:
MDEV-29748 ASAN errors or server crash in File_parser::parse upon concurrent view operations
Read the version of the view share when we read definition to prevent simultaniouse access to a view table SHARE (and so its MEM_ROOT) from different threads.
diff --git a/sql/sql_view.cc b/sql/sql_view.cc index 68a8410f559..1e4a740f871 100644 --- a/sql/sql_view.cc +++ b/sql/sql_view.cc @@ -1175,19 +1175,24 @@ static int mysql_register_view(THD *thd, TABLE_LIST *view, bool mariadb_view_version_get(TABLE_SHARE *share) { DBUG_ASSERT(share->is_view); + DBUG_ASSERT(share->tabledef_version.length == 0);
if (!(share->tabledef_version.str= (uchar*) alloc_root(&share->mem_root, MICROSECOND_TIMESTAMP_BUFFER_SIZE))) return TRUE; - share->tabledef_version.length= 0; // safety if the drfinition file is brocken
DBUG_ASSERT(share->view_def != NULL); if (share->view_def->parse((uchar *) &share->tabledef_version, NULL, view_timestamp_parameters, 1, &file_parser_dummy_hook)) + { + // safety if the definition file is brocken + share->tabledef_version.length= 0;
when can view_def->parse() fail? _Only_ if the definition is invalid? Can it succeed for one TABLE and fail for another, both of the same TABLE_SHARE?
Also if there is no memory. If for the other table there will be more memory or file has changed it can succeed.
return TRUE; + } DBUG_ASSERT(share->tabledef_version.length ==
MICROSECOND_TIMESTAMP_BUFFER_SIZE-1);
+ return FALSE; }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Oleksandr, On Oct 19, Oleksandr Byelkin wrote:
On Wed, Oct 19, 2022 at 5:09 PM Sergei Golubchik <serg@mariadb.org> wrote:
On Oct 19, Oleksandr Byelkin wrote:
if (share->view_def->parse((uchar *) &share->tabledef_version, NULL, view_timestamp_parameters, 1, &file_parser_dummy_hook)) + { + // safety if the definition file is brocken + share->tabledef_version.length= 0;
when can view_def->parse() fail? _Only_ if the definition is invalid? Can it succeed for one TABLE and fail for another, both of the same TABLE_SHARE?
Also if there is no memory.
If for the other table there will be more memory or file has changed it can succeed.
My point is that if it is possible that two different threads would get different results from share->view_def->parse(), then you cannot update the shared share->tabledef_version.length without a lock. And you cannot read it without a lock. So if it is possible that two different threads would get different results from share->view_def->parse(), then it's best not to update share->tabledef_version.length here and only do it in open_table_def(). Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Sergei! On Thu, Oct 20, 2022 at 6:23 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Oleksandr,
On Oct 19, Oleksandr Byelkin wrote:
On Wed, Oct 19, 2022 at 5:09 PM Sergei Golubchik <serg@mariadb.org> wrote:
On Oct 19, Oleksandr Byelkin wrote:
if (share->view_def->parse((uchar *) &share->tabledef_version,
NULL,
view_timestamp_parameters, 1, &file_parser_dummy_hook)) + { + // safety if the definition file is brocken + share->tabledef_version.length= 0;
when can view_def->parse() fail? _Only_ if the definition is invalid? Can it succeed for one TABLE and fail for another, both of the same TABLE_SHARE?
Also if there is no memory.
If for the other table there will be more memory or file has changed it can succeed.
My point is that if it is possible that two different threads would get different results from share->view_def->parse(), then you cannot update the shared share->tabledef_version.length without a lock. And you cannot read it without a lock.
So if it is possible that two different threads would get different results from share->view_def->parse(), then it's best not to update share->tabledef_version.length here and only do it in open_table_def().
if we got an error as I understood it will nead to an error of opening the table, so the query will fail. The other thread may have memory but it is true for any EOM situation and there are a lot of places where we allocate memory, so I do not see how this will differ. The same with changing frm file with a working server, I do not think that our server will survive if something out of the server will change frms.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Oleksandr, On Oct 21, Oleksandr Byelkin wrote:
if (share->view_def->parse((uchar *) &share->tabledef_version, NULL, view_timestamp_parameters, 1, &file_parser_dummy_hook)) + { + // safety if the definition file is brocken + share->tabledef_version.length= 0;
if we got an error as I understood it will nead to an error of opening the table, so the query will fail.
The other thread may have memory but it is true for any EOM situation and there are a lot of places where we allocate memory, so I do not see how this will differ.
The same with changing frm file with a working server, I do not think that our server will survive if something out of the server will change frms.
May be not, but it doesn't mean we need to introduce intentionally more places that can cause a crash. What will happen if you would not set share->tabledef_version.length= 0 there? Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Oleksandr Byelkin -
Sergei Golubchik