Base64::Decode に不正な引数のチェックを追加

[Raclamusi]

追加する機能の内容

• Base64::Decode に、不正なBase64文字列に対するエラー処理を追加する

その機能の追加によって解決する問題

• Base64::Decode に不正なBase64文字列を渡した場合、予期せぬ変換が行われる問題

問題が発生するコード例

# include <Siv3D.hpp> // OpenSiv3D commit 23f69823022c686ef995325e55d1664601a5d4b6

void DecodeAndPrint(StringView s)
{
	auto blob = Base64::Decode(s);
	Print << Unicode::FromUTF8(std::string_view{ reinterpret_cast<const char*>(blob.data()), blob.size() });
}

void Main()
{
	DecodeAndPrint(U"U2l2M0Q");

	DecodeAndPrint(U"I*CTi7Twk4eL8JOGkfCTj7zwk4I*p1w");  // "L/CTi7Twk4eL8JOGkfCTj7zwk4KnXAA" として変換

	DecodeAndPrint(U"けべびひ");  // "Qysr" として変換

	while (System::Update());
}

実行結果

Siv3D
/𓋴𓇋𓆑𓏼𓂧\
C++

備考

良さそうならPRを立てるつもりです。

[Reputeless]

具体的にはどのような実装を考えていますか。 実行時性能に悪い影響が出ると思うので、Base64::Decode() の引数に s3d::SkipValidation を追加するのが良さそうです。

[Raclamusi]

実装はこんな感じで考えています。

https://github.com/Raclamusi/OpenSiv3D/commit/6e5019794fd2c3e810c79806bcbaa84b20f6a95d https://github.com/Raclamusi/OpenSiv3D/commit/ce65fe92c4074497213f078b364e1db50ba043b7

[Reputeless]

ご提案ありがとうございます。コードをレビューして返信します。少々お待ちください。

[Raclamusi]

SkipValidation を追加するついでに、ほかの実装を考えました。

SivBase64.cpp の実装案 (GitHub Gist)

上から、始めにすべてチェックしてしまう実装 (SivBase64_CheckAtFirst.cpp) 、変換のタイミングでチェックをする実装 (SivBase64_CheckWhileConverting.cpp) と、それを constexpr if を使うことで改良した実装 ( SivBase64_CheckWhileConverting_UsingConstexprIf.cpp) です。

処理速度はだいたい次の表のような感じです。

	初めにチェック	変換しながらチェック	変換しながらチェック(constexpr if)
チェックありの処理速度	遅	速	速
チェックなしの処理速度	速	遅	速

---

適当に実行時間を測定した結果も載せておきます。

測定用コード（折り畳み）

# include <Siv3D.hpp>

void Main()
{
	constexpr int iteration = 100'000;
	String text = U"SGVsbG8sIE9wZW5TaXYzRCEg";
	String base64;
	Stopwatch stopwatch;

	for ([[maybe_unused]] int i : step(5))
	{
		text += text;
	}

	Console << U"base64.size()\tSkipValidation::No\tSkipValidation::Yes";

	std::this_thread::sleep_for(1s);  // 安定するまで少し待つ

	for ([[maybe_unused]] int i : step(10))
	{
		base64 += text;

		stopwatch.restart();
		for ([[maybe_unused]] int j : step(iteration))
		{
			(void)Base64::Decode(base64);
		}
		stopwatch.pause();
		const auto time1 = stopwatch.us();

		stopwatch.restart();
		for ([[maybe_unused]] int j : step(iteration))
		{
			(void)Base64::Decode(base64, SkipValidation::Yes);
		}
		stopwatch.pause();
		const auto time2 = stopwatch.us();

		Console << U"{}\t{}\t{}"_fmt(base64.size(), time1, time2);
	}

	while (System::Update());
}

チェックあり (SkipValidation::No) での処理時間

チェックなし (SkipValidation::Yes) での処理時間

実行時間だけ見れば、 constexpr if を使った変換しながらチェックする実装が最高ですね。 入力チェックしてるのに今までの実行時間と大差ないですし、チェックをしなければ倍近く速くなります。

コードはあまりきれいとは言えませんが。

---

計測環境

項目	環境
プロセッサ	Intel(R) Core(TM) i5-10210U CPU @ 1.60GHz 2.11 GHz
RAM	8.00 GB
OS	Windows 11 Home
IDE	Microsoft Visual Studio Community 2022 (64 ビット) 17.2.5
ビルド	Debug x64 （1回だけ Release で試したが特に変わらなかったので）

[Raclamusi]

かなり期間が空いたので一度まとめます。

現在の Base64::Decode の実装の問題点

https://github.com/Siv3D/OpenSiv3D/blob/b6b652461611389ff4e836f276e810126739e06f/Siv3D/src/Siv3D/Base64/SivBase64.cpp#L141-L165

現在 (v0.6.6) の Base64::Decode における Base64 文字列の正当性チェックは、 $4i+2$ 文字目と $4i+3$ 文字目についてのみ行うという中途半端な実装になっています。 また、文字コードが 256 以上の文字については下位 8 ビットしか見ていません。

これにより、I*CTi7Twk4eL8JOGkfCTj7zwk4I*p1w や けべびひ といった不正な Base64 文字列をデコードできてしまいます。

提案

すべての文字について Base64 文字列として正しいかどうかのチェックを実装します。 また、Base64::Decode の引数に SkipValidation を追加して正当性チェックの有無を選択できるようにし、チェックしない場合は実装からチェックを完全に削ってより高速に動作するようにします。

実装

実装案を以下の GitHub Gist に示します。

https://gist.github.com/Raclamusi/0834cd9eb85c87cffdc87bd0483f7ca5

結果

不正な Base64 文字列が変換されなくなりました。

# include <Siv3D.hpp>

void DecodeAndPrint(StringView s)
{
	auto blob = Base64::Decode(s);
	Console << U"[{}]"_fmt(Unicode::FromUTF8(std::string_view{ reinterpret_cast<const char*>(blob.data()), blob.size() }));
}

void Main()
{
	DecodeAndPrint(U"U2l2M0Q");
	DecodeAndPrint(U"I*CTi7Twk4eL8JOGkfCTj7zwk4I*p1w");
	DecodeAndPrint(U"けべびひ");

	while (System::Update())
	{
	}
}

実行結果

[Siv3D]
[]
[]

また、デコードする Base64 文字列の長さによる実行時間は以下のようになりました。

計測環境

プロセッサ	Intel(R) Core(TM) i5-10210U CPU @ 1.60GHz 2.11 GHz
RAM	8.00 GB
OS	Windows 11 Home
IDE	Microsoft Visual Studio Community 2022 (64 ビット) 17.5.1

相談

不正な文字を検出したときの処理は、空の Blob を返すか、 Error を投げるか、どちらがいいでしょうか。

[Reputeless]

ありがとうございます。ご提案の方針で概ね良さそうです。 v0.6.7 で受け入れるよう進めます。

例外 or 空について

エラーがわりと起こりがちであれば「空」を選択します。

• 例: Texture で画像ファイルが見つからない、ロードに失敗する

一方今回のケースでは

• 不正な Base64 入力からのデコードという、あってはいけない異常事態寄りのエラー
• 正常な空データとの区別

のために例外を選択したいと思います。 下記のような関数を SivBase64.cpp に用意して、必要な箇所で呼んでください。

[[noreturn]]
static void ThrowBase64DecodeError(const size_t i)
{
	throw Error{ U"Base64::Decode(): Invalid character at {}"_fmt(i) };
}

ヘッダのドキュメントにも

/// @throw Error 妥当性チェックが有効で、不正な入力が見つかった場合

のようなコメントを追加してください。

コードとコメントは思い付きの例なので、完全に同じにする必要はありません。
Parse.hpp / Parse.ipp に似た例があります。

[Raclamusi]

きれいなコードを書くならこっちの実装のほうがいいと思うのですが、実行速度とコードの美しさはどちらを優先したらいいでしょうか。 読みやすいコードを書きたい気持ちもありますが、変更前とあまり性能差を出したくないという気持ちもあります。

[Reputeless]

Base64 は頻繁にメンテナンスされる機能ではないので、速度優先で良いと思います。 以下のような変更も考えられます。

• std::size(decodeTable) に名前を付ける
• 次のようにまとめる（性能にマイナスになる可能性もあり）

if (255 < (pSrc[0] | pSrc[1] | pSrc[2] | pSrc[3]))
  Throw(pSrcBegin, pSrc);

const uint8 v1 = decodeTable[static_cast<uint8>(pSrc[0])];
const uint8 v2 = decodeTable[static_cast<uint8>(pSrc[1])];
const uint8 v3 = decodeTable[static_cast<uint8>(pSrc[2])];
const uint8 v4 = decodeTable[static_cast<uint8>(pSrc[3])];

if ((v1 | v2 | v3 | v4) == 0xff)
  Throw(pSrcBegin, pSrc);

pSrc += 4;

[Raclamusi]

提案コードを更新しました。

https://gist.github.com/Raclamusi/0834cd9eb85c87cffdc87bd0483f7ca5

• ヘッダの Doxygen コメントを追加しました
• 末尾の不正な文字が無視される問題を解決しました
• 不正な文字を検出したときに例外を搬出するようにしました
• 長さが不正であったときにも例外を搬出するようにしました
• std::size(decodeTable) を 256 に置き換えました
• 警告抑制の方法をキャスト ( static_cast<std::size_t>(*pSrc) ) から constexpr if ( if constexpr (sizeof(Ch) >= 2) ) に変更しました
• 上のコメントでご提案頂いたコードは実行速度への影響が大きかったため採用しませんでした

[Reputeless]

ありがとうございます。上記提案をベースに Pull-request を作成してください。 v0.6.7 でのマージに向けて作業を進めます。