文字列比較

新人OJTネタです。

コーディングレビューでの一コマ。

今のプロジェクトでは、複数のプログラム言語を併用した開発を行っております。

今回は、C++でのコーディングで、コマンドのオプションの拡張。

オプションを追加することになり、その実装を担当させることになり、実装工程も終わり、コーディングレビューの工程へ。

オプションを追加して、そのオプション名のチェック処理ですが、あがってきたソースはこんな感じ。

if (strncmp(buf, OPTION_NAME_STR, sizeof(OPTION_NAME_STR)-1) == 0) {
     // オプション名が一致した場合の処理
}

引数ごとにbuf 変数に値を格納し、そのbuf変数の値がオプション名に一致しているかをチェックする処理ですが…

よくよく見るとこれはおかしいですよね。

■ strncmp関数

int strncmp(const char *s1, const char *s2, size_t n);

文字列*s1 と文字列*s2 を先頭からn文字比較。'\0'以降の比較は行われない。

s1 > s2 で正の値が，s1 < s2 で負の値が，s1 = s2で 0 が返される。

sizeof(OPTION_NAME_STR) のサイズは '\0' までのサイズになるため、そのため、上記実装をよく見てみると、比較するサイズを sizeof(buf) - 1 とすると’\0' は比較対象に含まれないことになります。

仮に

#define OPTiON_NAME_STR   "option"

とマクロ定義していたとすると、strncmp関数で sizeof(OPTION_NAME_STR) で比較されるのは '\0' ですが、「-1」しているために 'n' までになってしまいます。

こうすると、完全一致判定条件ではなく前方一致判定条件になってしまい、「option」で始まっている文字列であれば、何でも条件を満たしてしまいます。

例えば buf変数に「option2」とか「option test」とかが入っている場合でも、条件に当てはまってしまいます。

本人にこの実装おかしくない？と聞いてみると、「？？？？」という感じでした。

他に同じような処理があり、それを持ってきて実装してみてとりあえず動いたので大丈夫みたいな感じだったようですね。

なんで「-1」したのかについては、

strncpy (buf, tmp, sizeof(buf) - 1);

があったからのようです。

strncpy関数で、コピー先の領域サイズから１引く理由は、strncpy関数は '\0' は自動付与してくれないので、sizeof(tmp) > sizeof(buf) の場合、buf変数の最後に '\0' が付与されないままになってしまいます。

このまま参照してしまうと危ないですね。

コーディング時間を短縮するためには、既存のコードを流用するというのは勿論アリだと思います。

ただ、何も考えずにコピペしてもダメで、その処理をきちんと理解した上で流用しているかどうかになりますね。