この記事はOpenCV Advent Calendar 2022の1日目の記事です。
他の記事は目次にまとめられています。
TL;DR
-
for(int i=0;i<params.size();i+=2)は(全て条件次第で）out-of-boundary起こすかも。 - 13年前からの潜在バグが、9年前からの潜在バグで直せない...
簡単に自己紹介？
画像処理系の組み込みエンジニアです。OpenCV communityにもちょくちょくコメントしています。以後、よろしくお願いします！
13年前からの潜在バグ
以下のコードに違和感を感じ、調査を進めた。ということで、この時点でどんなバグが含まれているのか、予想できますでしょうか？ここでピピっと来た方は非常に鋭いと思います。
grft_jpeg.cpp
for( size_t i = 0; i < params.size(); i += 2 )
{
if( params[i] == IMWRITE_JPEG_QUALITY )
{
quality = params[i+1];
quality = MIN(MAX(quality, 0), 100);
}
if( params[i] == IMWRITE_JPEG_PROGRESSIVE )
{
progressive = params[i+1];
}
:
}
仕様を確認する
imwriteのparamsの仕様を先に確認しておこう。 paramsは「ペアで入れる」と明記されている。
Parameters
filename Name of the file.
img (Mat or vector of Mat) Image or Images to be saved.
params Format-specific parameters encoded as pairs
(paramId_1, paramValue_1, paramId_2, paramValue_2, ... .)
see cv::ImwriteFlags
正常系を考える
例えば、下記属性を引数として渡したい場合を考える。
| ID | VALUE |
|---|---|
| IMWRITE_JPEG_QUALITY | 100 |
| IMWRITE_JPEG_LUMA_QUALITY | 85 |
| IMWRITE_JPEG_CHROMA_QUALITY | 75 |
ユーザーアプリケーション側の実装は以下となるだろう。
user application
std::vector<int> params;
params.push_back( IMWRITE_JPEG_QUALITY );
params.push_back( 100 );
params.push_back( IMWRITE_JPEG_LUMA_QUALITY );
params.push_back( 85 );
params.push_back( IMWRITE_JPEG_CHROMA_QUALITY );
params.push_back( 75 );
imwrite("XXX.jpg", img, params );
/*
* params = { IMWRITE_JPEG_QUALITY, 100,
* IMWRITE_JPEG_LUMA_QUALITY, 85,
* IMWRITE_JPEG_CHROMA_QUALITY, 75 };
* params.size() = 6;
*/
このデータに対して、ソースコードの動きをトレースしていく。
for( size_t i = 0; i < params.size(); i += 2 )
i=0, 2, 4 と、params.size()を越えるまでループする。 i=6となったら、条件を満たさなくなり、終了。
if( params[i] == IMWRITE_JPEG_QUALITY )
quality = params[i+1];
-
params[i]の中身が、当該Encoderに興味があるIDであるかを判別する。 - 興味があるIDだった場合、
params[i+1]の中身と紐づけて利用する。
params[0] = IMWRITE_JPEG_QUALITY;
params[1] = 100;
-> quality = 100;
params[2] = IMWRITE_JPEG_LUMA_QUALITY;
params[3] = 85;
-> luma_quality = 85;
params[4] = IMWRITE_JPEG_CHROMA_QUALITY;
params[5] = 75;
-> chroma_quality = 75;
そう、ここまではいいのだ…… ここまでは。だが、忘れてはいないだろうか、このparamsは「ユーザーアプリケーションが指定する」のだ。
異常系を考える
我々は潜在的に、「paramsは、IDとVALUEのペアで来るもの」 と思い込んだ。だが、これは常に真か？具体例を挙げてみる。もし、paramsにIDのみが入り、VALUEが入っていない場合はどうなるのか？
user application
std::vector<int> params;
params.push_back( IMWRITE_JPEG_QUALITY );
imwrite("XXX.jpg", img, params );
/*
* params = { IMWRITE_JPEG_QUALITY };
* params.size() = 1;
*/
先頭のIDに紐づいたVALUEを見に行こうとすると… out-of-boundaryが発生してしまう！ アイヤー！
params[0] = IMWRITE_JPEG_QUALITY;
params[1] = ...; << Out-of-boundary!!!>
実際に問題を引き起こしてみよう。ubuntu 22.10環境でサクッとvalgrindで境界外アクセスを検出してみた。
/**
* $ g++ main.cpp -o a,out -I /usr/local/include/opencv4 -lopencv_core -lopencv_imgcodecs
* $ valgrind ./a.out
*/
#include <opencv2/core.hpp>
#include <opencv2/imgcodecs.hpp>
#include <string>
#include <vector>
using namespace std;
using namespace cv;
int main(void)
{
const Mat img( 16, 16, CV_8UC3, cv::Scalar::all(0) );
vector<int> params;
params.push_back(IMWRITE_JPEG_QUALITY);
// params.push_back(100)); // Forget it.
cv::imwrite("test.jpg", img, params);
}
テスト結果
[ RUN ] Imgcodecs.write_parameter_length
==103191== Invalid read of size 4
==103191== at 0x48A6D80: cv::JpegEncoder::write(cv::Mat const&, std::vector<int, std::allocator<int> > const&) (grfmt_jpeg.cpp:647)
==103191== by 0x487ED7A: cv::imwrite_(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<cv::Mat, std::allocator<cv::Mat> > const&, std::vector<int, std::allocator<int> > const&, bool) (loadsave.cpp:xxx)
==103191== by 0x487F48F: cv::imwrite(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cv::_InputArray const&, std::vector<int, std::allocator<int> > const&) (loadsave.cpp:xxx)
このバグはいつから入り込んだのか？
githubにOpenCVが移行した時点でのコードには既にあった(2010年5月12日)。
Github移行当初のgrft_jpeg.cpp
for( size_t i = 0; i < params.size(); i += 2 )
{
if( params[i] == CV_IMWRITE_JPEG_QUALITY )
{
quality = params[i+1];
quality = MIN(MAX(quality, 0), 100);
}
}
Githubに移行する前のバージョンのソースコードも、sourceforgeから入手できる。
ということで確認した結果は、OpenCV 2.0で、params対応を入れたときからずーっと暖められていましたね. ということで、2009/10/1から13年間対応されてこなったバグになります。
9年前の潜在バグ
さあ、これで問題解決！！となるかと思ったら、もうちょっとだけ続いたんじゃ。
そうは問屋が卸さなかった
もともとparamsの長さチェックするコードは存在していたので、ここに追加でいいかな？と思っていました。
loadsave.cpp
static bool imwrite_( const String& filename, const std::vector<Mat>& img_vec,
const std::vector<int>& params, bool flipv )
{
bool isMultiImg = img_vec.size() > 1;
std::vector<Mat> write_vec;
＜略＞
encoder->setDestination( filename );
CV_Assert(params.size() <= CV_IO_MAX_IMAGE_PARAMS*2); // ★ココ
bool code = false;
try
{
if (!isMultiImg)
code = encoder->write( write_vec[0], params );
else
code = encoder->writemulti( write_vec, params ); //to be implemented
だが CV_Assert( (params.size() % 2) == 0 ); を単純追加したところ、imgcodecsのregressionテストに失敗するようになる。 アイヤー! その原因は無法者がいたのじゃ…
HDR Encoderの設計ミス
テストで問題になっているコードを見ると…ダメじゃんかコレ、と絶望しました。まさしく「１つしかデータを詰め込んでいない」という問題あるコード…あれ？
#ifdef HAVE_IMGCODEC_HDR
TEST(Imgcodecs_Hdr, regression)
{
＜略＞
vector<int>param(1);
for(int i = 0; i < 2; i++) {
param[0] = i;
imwrite(tmp_file_name, img_rle, param);
Mat written_img = imread(tmp_file_name, -1);
ASSERT_FALSE(written_img.empty()) << "Could not open " << tmp_file_name;
minMaxLoc(abs(img_rle - written_img), &min, &max);
ASSERT_FALSE(max > DBL_EPSILON);
}
remove(tmp_file_name.c_str());
}
#endif
そうするとHDR Encoderの実装を見ると、imwrite()のparamsの説明とあっていないのである。HDR_NONE？HDR_RLE？あなたたちはどこから来たのですか？状態。
bool HdrEncoder::write( const Mat& input_img, const std::vector<int>& params )
{
＜略＞
CV_Assert(params.empty() || params[0] == HDR_NONE || params[0] == HDR_RLE);
FILE *fout = fopen(m_filename.c_str(), "wb");
if(!fout) {
return false;
}
RGBE_WriteHeader(fout, img.cols, img.rows, NULL);
if(params.empty() || params[0] == HDR_RLE) {
RGBE_WritePixels_RLE(fout, const_cast<float*>(img.ptr<float>()), img.cols, img.rows);
} else {
RGBE_WritePixels(fout, const_cast<float*>(img.ptr<float>()), img.cols * img.rows);
}
本来は、ImwriteFlagsに、IMWRITE_HDR_COMPRESSIONを追加するべきでしたね…
//! Imwrite flags
enum ImwriteFlags {
IMWRITE_JPEG_QUALITY = 1, //!< For JPEG, it can be a quality from 0 to 100 (the higher is the better). Default value is 95.
IMWRITE_JPEG_PROGRESSIVE = 2, //!< Enable JPEG features, 0 or 1, default is False.
IMWRITE_JPEG_OPTIMIZE = 3, //!< Enable JPEG features, 0 or 1, default is False.
IMWRITE_JPEG_RST_INTERVAL = 4, //!< JPEG restart interval, 0 - 65535, default is 0 - no restart.
IMWRITE_JPEG_LUMA_QUALITY = 5, //!< Separate luma quality level, 0 - 100, default is -1 - don't use.
IMWRITE_JPEG_CHROMA_QUALITY = 6, //!< Separate chroma quality level, 0 - 100, default is -1 - don't use.
IMWRITE_JPEG_SAMPLING_FACTOR = 7, //!< For JPEG, set sampling factor. See cv::ImwriteJPEGSamplingFactorParams.
IMWRITE_PNG_COMPRESSION = 16, //!< For PNG, it can be the compression level from 0 to 9. A higher value means a smaller size and longer compression time. If specified, strategy is changed to IMWRITE_PNG_STRATEGY_DEFAULT (Z_DEFAULT_STRATEGY). Default value is 1 (best speed setting).
IMWRITE_PNG_STRATEGY = 17, //!< One of cv::ImwritePNGFlags, default is IMWRITE_PNG_STRATEGY_RLE.
IMWRITE_PNG_BILEVEL = 18, //!< Binary level PNG, 0 or 1, default is 0.
IMWRITE_PXM_BINARY = 32, //!< For PPM, PGM, or PBM, it can be a binary format flag, 0 or 1. Default value is 1.
IMWRITE_EXR_TYPE = (3 << 4) + 0, /* 48 */ //!< override EXR storage type (FLOAT (FP32) is default)
IMWRITE_EXR_COMPRESSION = (3 << 4) + 1, /* 49 */ //!< override EXR compression type (ZIP_COMPRESSION = 3 is default)
IMWRITE_WEBP_QUALITY = 64, //!< For WEBP, it can be a quality from 1 to 100 (the higher is the better). By default (without any parameter) and for quality above 100 the lossless compression is used.
IMWRITE_PAM_TUPLETYPE = 128,//!< For PAM, sets the TUPLETYPE field to the corresponding string value that is defined for the format
IMWRITE_TIFF_RESUNIT = 256,//!< For TIFF, use to specify which DPI resolution unit to set; see libtiff documentation for valid values
IMWRITE_TIFF_XDPI = 257,//!< For TIFF, use to specify the X direction DPI
IMWRITE_TIFF_YDPI = 258,//!< For TIFF, use to specify the Y direction DPI
IMWRITE_TIFF_COMPRESSION = 259,//!< For TIFF, use to specify the image compression scheme. See libtiff for integer constants corresponding to compression formats. Note, for images whose depth is CV_32F, only libtiff's SGILOG compression scheme is used. For other supported depths, the compression scheme can be specified by this flag; LZW compression is the default.
IMWRITE_JPEG2000_COMPRESSION_X1000 = 272 //!< For JPEG2000, use to specify the target compression rate (multiplied by 1000). The value can be from 0 to 1000. Default is 1000.
};
このバグはいつから入り込んだのか？
HDR Encoderをサポートした2013年7月4日の時点からずっと同じバグが... OpenCV 3.0からの潜在障害？ですかね！！困ったもんだ...
それからどうなった？
OpenCV Communityで直していただける運びとなりました！！ひゃっほーい！！ありがとうございます、ありがとうございます！！
こんな感じのなかなかアグレッシブな？コードになりました！
encoder->setDestination( filename );
#if CV_VERSION_MAJOR < 5 && defined(HAVE_IMGCODEC_HDR)
bool fixed = false;
std::vector<int> params_pair(2);
if (dynamic_cast<HdrEncoder*>(encoder.get()))
{
if (params_.size() == 1)
{
CV_LOG_WARNING(NULL, "imwrite() accepts key-value pair of parameters, but single value is passed. "
"HDR encoder behavior has been changed, please use IMWRITE_HDR_COMPRESSION key.");
params_pair[0] = IMWRITE_HDR_COMPRESSION;
params_pair[1] = params_[0];
fixed = true;
}
}
const std::vector<int>& params = fixed ? params_pair : params_;
#else
const std::vector<int>& params = params_;
#endif
CV_Check(params.size(), (params.size() & 1) == 0, "Encoding 'params' must be key-value pairs");
CV_CheckLE(params.size(), (size_t)(CV_IO_MAX_IMAGE_PARAMS*2), "");
まとめ
- ドキュメントに「ペアで扱う」と書いていても、実装がそうなってないかもしれない。
- テストはしっかり書こう！！レグレッションテストを13年間スルーしてきたというのは怖いね！
以上となります、ご精読ありがとうございました！！
12月2日は、@UnaNancyOwen さんの「OpenCVのdnnモジュールのクラス分類で信頼度を[0.0-1.0]にする」のです！
それでは、今年も何卒、くれぐれも、よろしくお願いいたします！！
（おまけ）この問題に気が付いた時の私のコメント...
imwrite()「パラメータは、ID, VALUEのpairで扱います」
— 恋する熊太郎さん＠次回ガブガブは明日！！ (@hon_no_mushi) November 5, 2022
HDR Encoder「先頭要素をVALUEとしてみるでー」
ｺﾞﾗｱ