C言語のバグ回避をするための習慣

概要

C言語でコーディングする上で気をつけている点などをまとめて見ました。
但し、書き方は人それぞれなので違和感を覚える人もいるかもしれませんが、
もし間違っている点がありましたらご指摘お願い致します。

目的

C言語について文法はある程度理解はしたが、その先がわからない。
という人向けにこんな感じでコーディングすればバグが減るかも。
という指針的なものを提供したかったです。

条件判定時の習慣

基本的にはテストを行う部分ではありますが、急いでいたりすると
意外な盲点に気付かずにそのままスルーしてしまう事がよくあります。
多少面倒かもしれませんがちょっと書き方を工夫する事でミスを事前に回避できます。

変数の位置

条件判定の時、変数を左に書きたくなります。皆さんその方が理解しやすいと思います。
でも以下の書き方はやらない方が良いです。
何故ならうっかりミスで==を=にしてしまった時に気付かない可能性があるからです。

#define　RET_OK 0
if(ret == RET_OK) {
  func();
}

if (val = RET_OK) {  // うっかりミス
  func();
}

上のように書いてもコンパイラは文句を言わない。
→-Wallオプション(警告を全て出力するというコンパイル時の指定)を付けると
　コンパイルから文句が発するようになるがコンパイル自体は
　通って(成功して実行ファイルを生成)しまいます。

実行するとvalに0が代入されてfunc()がコールされる事はありません。
これを回避するには数値を左側に書くと良いです。

better

if (RET_OK == val) {
  func();
}

上記の場合うっかり=を一個忘れた場合、コンパイル実行時に
コンパイルエラーとなり、コンパイルを中断してくれるので間違いに気づくかと思います。

NULL参照

関数の引数をチェックとかする場面があると思います。
その際にif文の中の処理順序まで考慮すべきです。

void func(strct aaa *prm ) {
  if((prm->a == NULL) || (NULL == prm)) {
    return RET_NG;
  }
  prm->a = 100;
  return RET_OK;
}

条件判定は左から順に判定されます。
この場合(prm->a == NULL)が先に判定されます。
もしprmがNULLの場合、prm->aを参照できずプログラムは異常停止してしまいます。
判定が複数ある場合、順序を意識してNULLを参照しないように書くべきです。

better

void func(strct aaa *prm) {
  if ((NULL == prm) || (prm->a == NULL)) {
    return RET_NG;
  }
  prm->a = 100;
  return RET_OK;
}

因みにですが、論理演算子を&& (○○かつ××)にしてしまった場合、
2つの条件は必ず実行されてしまいます。
そのためprmがNULLの場合に2つ目の条件でNULL参照となってしまいます。
|| (○○または××)にする事でprmがNULLの場合でも、(NULL == prm) を判定した時点でif()の結果はtrueとなり、
その後に続く判定(prm->a == NULL)は行われませんのでNULL参照を回避できます。

初期化忘れを回避する習慣

C言語では変数を初期化しないとその変数の初期値はゴミ(めちゃめちゃな値)が入っています。
何の値が入っているかわからないのでゴミ変数をそのまま使うと大抵おかしな挙動となってしまいます。
なので変数は必ず初期値を設定し、初期化を行う習慣をつけるべきです。

変数の連続定義はしない

初期値は特に決まってないなら値は0を代入しとけば良いかと思います(ポインタでしたらNULL)。
0で初期化したつもりが実はされていなかった、という場合が有り得ます。

int a,b,c = 0;

一見良さそうに見えますがこの書き方をやってしまうと変数のaとbは0で初期化されません。
面倒でも1つずつ初期化すべきです。

better

int a = 0;
int b = 0;
int c = 0;

配列、構造体の中身

配列や構造体を初期化したい場合要素1つ1つ初期化するのは面倒だ
なるべく楽をしたい。例えばこんな方法で初期化を行う。

int func() {
  char aaa[10];
  memset( &aaa[0] , 0x00 , sizeof(aaa) );
  return 0;
}

変数aaaの初期化にmemsetを使用していますが、memset()は必ずしも全てのマシンで
同じような初期化が行われるとは限らないようです。
出来るだけ処理系依存を気にしないやり方を選びたい場合初期化は以下にすると無難です。

better

int func(){
  char aaa[10] = {0};
  return 0;
}

sizeofの指定

sizeof()に限らず、直接数値を指定するような書き方は
後から何の意味を持った数値かがわからなくなるので避けるべきです。
sizeof()の場合、型を指定するべきですがそれを習慣としているが故にミスる時が有ります。

bug

void func() {
  int loop = 0;
  struct aaa *aaa = NULL;
  aaa = (struct aaa *) malloc(sizeof(struct aaa *));
  if( NULL == aaa ) {
    return RET_NG;
  }
  // 何か処理
  free(aaa);
  return RET_OK;
}

malloc時に指定したサイズに構造体の型ではなくポインタ型を指定しています。
渡すべきはmallocによって割り当てられたメモリー領域へのポインタ型の大きさではなく、割り当てたい大きさ、すなわち要素型(ここではstruct aaa)です。
したがって上記例はバグです。

OK

void func() {
  int loop = 0;
  struct aaa *aaa = NULL;
  aaa = malloc(sizeof(struct aaa));  // もしくはsizeof(*aaa)
  if (NULL == aaa) {
    return;
  }
  // 何か処理
  free(aaa);
  return;
}

変数型を扱う際の習慣

C言語のには同じ種類の型の中で符号あり/なしがあります。
これらを分けて扱う必要があります。

関数からの戻り値

unsigned char get_val () {
    return -1;
}
void main() {
    if(0 > get_val()) {
        printf("マイナスだよ。\n");
    } else {
        printf("プラスだよ。\n");
    }
}

上記の場合 (0 > val) の判定は常にfalseとなります。
get_val側で-1を返していても関数としては符号なしのchar型として返すので、
返却値は255(char型の最大値)になります。
書き方の習慣というよりは、関数仕様をきちんと確認する習慣を付けるべきと言えるかもしれません。

終端文字の考慮

C言語の文字列は終端文字を考慮しなければなりませんが、
考慮した結果、意図しない動作をする事があります。
以下のコードは文字列分のmallocを行おうとしていますが、
きちんと終端文字を考慮して文字列の長さ+1文字分の領域確保を試みております。

void func(char *pstr) {
  char len = 0;
  char *str = null;

  if (NULL == pstr) {
      return RET_NG;
  }
  len = strnlen(pstr, 254);
  if ( 0 < len ) {
    str = (char *) malloc(len + 1);
    if (NULL == str) {
      return RET_NG;
    }    
    // 省略
    free(str);
  }
  return RET_OK;
}

上記のコードではget_len()がint型が取りうる最大値(例えば255)を返した時、
mallocする時の len+1 は限界を突破してマイナス値(-1)となってしまいます。
終端文字などを考慮する際には、型の最大値などに注意する習慣を付けると良いかと思います。

better

void func(char *pstr) {
  char len = 0;
  char *str = null;

  if (NULL == pstr) {
      return RET_NG;
  }
  len = strnlen(pstr, 253);    // 終端文字とlenの最大値を考慮
  if ( 0 < len ) {
    str = (char *) malloc(len + 1);
    if (NULL == str) {
      return RET_NG;
    }    
    // 省略
    free(str);
  }
  return RET_OK;
}

文字列を扱う時の習慣

終端文字

(途中..)

配列要素のアクセス位置

初歩的な事ではありますが、配列の番地についての注意点になります。
以下のコードは配列の最後の要素に終端文字の挿入を試みていますが、
実行すると異常停止します。

str[100] = {0};
memcpy( str , param , sizeof(str) );
str[sizeof(str)] = '\0';

配列の番地は0から数えますので上記の例では存在しない
101番目の要素へのアクセスをしている事になります。
配列の最後の要素にアクセスする際は”sizeof()から-1”というような
自分なりの安全な書き方を覚えてしまうのも良いかもしれません。

better

str[100] = {0};
memcpy(str, param, sizeof(str));
str[sizeof(str)-1] = '\0';

型の大きさ

大きい型サイズから小さい型サイズにキャストした場合、情報が失われる場合があります。
(途中)

free漏れをなくす習慣

free漏れは厄介です。
個人的にfree漏れは一番嫌いなバグです。
free漏れを無くすコーディングを心がけて製造の段階でしっかりとfree漏れを無くした方が絶対に良いです。

エラールート

free()するのは関数の最後だけではありません。
エラールートにも気を使わないとfree漏れを起こします。

void func(query) {
  CONNECT *con = NULL;
  con = connect();
  if ( NULL == con ) {
    return;
  }
  if ( query( con , query) ) {
    return;
  }
  free(con);
  return;
}

better

void func (query) {
  CONNECT *con = NULL;
  con = connect();
  if( NULL == con ){
    return;
  }
  if ( query( con , query) ) {
    free(con);   // 忘れずに解放する!!
    return;
  }
  free(con);
  return;
}

地産地消

以下のコードはバグではありません。
しかし、どこでfreeされるかわからない変数を乱立させるとfree漏れの原因になりやすいです。
基本的にはmallocした関数内でfreeする事を推奨します。

int * getData() {
    int *ret_dat = NULL;
    // 省略
    ret_dat = (int *) malloc(dat_len);
    // 省略
    return ret_dat;
}
func () {
    int *dat = NULL;
    dat = getData();
    // 省略
    free(dat);
}

危険な関数

例に示すのは省略しますが、strlenやstrcmpを使うよりかは、
nがついている関数名(strnlenやstrncmpなど)を使った方が良いです。
nがついている関数の場合、自分で限界値を設定して関数の機能を使う事ができますので、
限界突破して暴走する心配が減ります。