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

この記事は[OpenCV Advent Calendar 2022](https://qiita.com/advent-calendar/2022/opencv)の1日目の記事です。
他の記事は[目次](https://qiita.com/advent-calendar/2022/opencv)にまとめられています。

## TL;DR
- `for(int i=0;i<params.size();i+=2)`は(全て条件次第で）out-of-boundary起こすかも。
- 13年前からの潜在バグが、9年前からの潜在バグで直せない...

## 簡単に自己紹介？
画像処理系の組み込みエンジニアです。OpenCV communityにもちょくちょくコメントしています。以後、よろしくお願いします！

## 13年前からの潜在バグ

以下の[コード](https://github.com/opencv/opencv/blob/21133a2091ba461d3351a4411cc6e46af564f2a4/modules/imgcodecs/src/grfmt_jpeg.cpp#L643-L655)に違和感を感じ、調査を進めた。ということで、この時点でどんなバグが含まれているのか、予想できますでしょうか？ここでピピっと来た方は非常に鋭いと思います。

```c++: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の仕様](https://docs.opencv.org/4.x/d4/da8/group__imgcodecs.html#gabbc7ef1aa2edfaa87772f1202d67e0ce)を先に確認しておこう。 paramsは「ペアで入れる」と明記されている。

```plaintext
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 |

ユーザーアプリケーション側の実装は以下となるだろう。

```c++: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]`の中身と紐づけて利用する。

```c++
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が入っていない場合はどうなるのか？

```c++: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が発生してしまう！ **アイヤー！**

```c++
params[0] = IMWRITE_JPEG_QUALITY;
 params[1] = ...; << Out-of-boundary!!!>
```

実際に問題を引き起こしてみよう。ubuntu 22.10環境でサクッとvalgrindで境界外アクセスを検出してみた。

```c++
/**
 * $ 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);
}
```

```bash:テスト結果
[ 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が移行した時点でのコード](https://github.com/opencv/opencv/blob/127d6649a1c83397bf42799ac807af41aa507b30/modules/highgui/src/grfmt_jpeg.cpp#L528-L534)には既にあった(2010年5月12日)。

```c++: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](
https://sourceforge.net/projects/opencvlibrary/files/opencv-unix/)から入手できる。

ということで確認した結果は、[OpenCV 2.0](https://sourceforge.net/projects/opencvlibrary/files/opencv-unix/2.0/)で、params対応を入れたときからずーっと暖められていましたね. ということで、2009/10/1から13年間対応されてこなったバグになります。

## 9年前の潜在バグ

さあ、これで問題解決！！となるかと思ったら、もうちょっとだけ続いたんじゃ。

### そうは問屋が卸さなかった

もともと[paramsの長さチェックするコード](https://github.com/opencv/opencv/blob/21133a2091ba461d3351a4411cc6e46af564f2a4/modules/imgcodecs/src/loadsave.cpp#L684-L727)は存在していたので、ここに追加でいいかな？と思っていました。

```c++: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の設計ミス

[テストで問題になっているコード](https://github.com/opencv/opencv/blob/21133a2091ba461d3351a4411cc6e46af564f2a4/modules/imgcodecs/test/test_grfmt.cpp#L335-L361)を見ると…ダメじゃんかコレ、と絶望しました。まさしく「１つしかデータを詰め込んでいない」という問題あるコード…あれ？

```c++
#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の実装](https://github.com/opencv/opencv/blob/21133a2091ba461d3351a4411cc6e46af564f2a4/modules/imgcodecs/src/grfmt_hdr.cpp#L131-L159)を見ると、imwrite()のparamsの説明とあっていないのである。`HDR_NONE`？`HDR_RLE`？あなたたちはどこから来たのですか？状態。

```c++
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](https://github.com/opencv/opencv/blob/4.x/modules/imgcodecs/include/opencv2/imgcodecs.hpp#L85-L107)に、`IMWRITE_HDR_COMPRESSION`を追加するべきでしたね…

```c++
//! 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日の時点](https://github.com/opencv/opencv/blob/a5e11079d743bf6bc4a31b1bd829e95cfa853bd2/modules/highgui/src/grfmt_hdr.cpp#L116-L123
)からずっと同じバグが...　OpenCV 3.0からの潜在障害？ですかね！！困ったもんだ...


## それからどうなった？

OpenCV Communityで直していただける運びとなりました！！ひゃっほーい！！ありがとうございます、ありがとうございます！！

https://github.com/opencv/opencv/pull/22830

こんな感じのなかなかアグレッシブな？コードになりました！

```c++
    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]にする](https://qiita.com/UnaNancyOwen/items/dc0a3c3afce60cd81d2f)」のです！

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

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

<blockquote class="twitter-tweet"><p lang="ja" dir="ltr">imwrite()「パラメータは、ID, VALUEのpairで扱います」<br><br>HDR Encoder「先頭要素をVALUEとしてみるでー」<br><br>ｺﾞﾗｱ</p>&mdash; 恋する熊太郎さん＠次回ガブガブは明日！！ (@hon_no_mushi) <a href="https://twitter.com/hon_no_mushi/status/1588854105782779904?ref_src=twsrc%5Etfw">November 5, 2022</a></blockquote> <script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>