MariaDBコントリビューション録その5 - MDEV-18873

前回のあらすじ

MDEV-24582に取り組むうちに、GDBが仲間に加わった。コードを実行しながら変数の中身も見えるすごいやつである。強力な仲間たちとともに、今日もIssueに立ち向かう。

tmokmss.hatenablog.com

とはいえ、とりあえず気持ちを変えてみる

ここ最近取り組んでいるMDEV-24582はなかなかの難敵なので、修正方針についてメンテナに合意を得てから実装に着手することにしよう。修正を追えてPRを出したあとに根本的な指摘をされるのはお互いに避けたいだろう。

該当のチケットに方針に関するたたき台を投稿したので、しばし返信を待つ。そしてこのままではブログネタがなくなるので、今日は別のIssueに取り組むことにする。

今回のIssue

特定のクエリを実行した際に、MariaDBサーバーがクラッシュするというバグ。クラッシュした際のスタックトレースが添付されているので、これを参考に見ていこう。

[MDEV-18873] Server crashes in Compare_identifiers::operator or in my_strcasecmp_utf8 upon ADD PERIOD IF NOT EXISTS with empty name - Jira

まずは例によって、問題が再現するクエリを書いたテストケースを作成し、実行してみる。

# mysql-test/suite/innodb/t/MDEV-18873.test
ALTER TABLE t ADD PERIOD IF NOT EXISTS FOR `` (s,e);

# run test
./mysql-test-run.pl innodb.MDEV-18873 --manual-gdb

ADD PERIOD という命令は初見だと思ったら、MariaDB特有の機能らしい。正直ほぼMySQL == MariaDBだと思っていたのだが、まれによく相違点があって面白い。

原因の特定

Issueのスタックトレースから Compare_identifiers::operator() でエラーが起きていることが分かるので、そこにブレークポイントを配置する。

#3  <signal handler called>
#4  0x000056225dec77ca in Compare_identifiers::operator() (this=0x7fd1a807a41f, a=..., b=...) at /data/src/10.4/sql/vers_string.h:42
#5  0x000056225ded034f in Lex_cstring_with_compare<Compare_identifiers>::streq (this=0x7fd198005e80, b=...) at /data/src/10.4/sql/vers_string.h:91
#6  0x000056225e0ec019 in LEX::add_period (this=0x7fd198004960, name=..., start=..., end=...) at /data/src/10.4/sql/sql_lex.h:4363
...

struct Compare_identifiers
{
  int operator()(const LEX_CSTRING& a, const LEX_CSTRING& b) const
  {
    DBUG_ASSERT(a.str[a.length] == 0);
    DBUG_ASSERT(b.str[b.length] == 0);
    return my_strcasecmp(system_charset_info, a.str, b.str);
  }
};

デバッガで追うと、 DBUG_ASSERT(a.str[a.length] == 0); でエラーが発生することが分かる。DebugビルドとReleaseビルドで挙動が異なるのも、このためだろう。今回のクエリでは、おそらくテーブル t が存在しないために、 a がNULLポインタになる模様。このため、 a.str がSegmatation faultとなり、サーバーがクラッシュする。

Releaseビルドでは、 my_strcasecmp の中でエラーが発生するが、これも a がNULLであることに起因するようなので、同じ原因と言えるだろう。

対処方法 - 1st trial

a.str を参照する前に、 a がNULLかどうか確認するのが最低限の対応となるだろう。以下のような判定を追加した。これまでは str がNULLの場合は必ず実行時エラーになっていたはずのため、この判定を追加しても既存の動作に悪影響はないはずだ。

  bool streq(const Lex_cstring_with_compare& b) const
  {
-    return Lex_cstring::length == b.length && 0 == Compare()(*this, b);
+    return Lex_cstring::length == b.length && str != NULL && 0 == Compare()(*this, b);
  }

やや対症療法的な感じはするが、シンプルで機能する修正ではあり、実際今回のエラーは解消する。一旦PRを作成して、レビュワーに確認してみることにする。

github.com