Go to Qiita Advent Calendar 2024 Top
LoginSignup
19
1

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

More than 1 year has passed since last update.

OpenCVAdvent Calendar 2022

Day 1
@hon_no_mushi

13年前からの潜在バグを修正しようとして、9年前の潜在バグに悩まされた話。

Last updated at Posted at 2022-11-30

この記事は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]にする」のです！

それでは、今年も何卒、くれぐれも、よろしくお願いいたします！！

（おまけ）この問題に気が付いた時の私のコメント...

19
1
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
19
1

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?