大変なことになっているプログラム
先日担当している工場のシステムリプレイス案件で、
どうしても設備にネットワーク接続できず、既存のソースプログラムを読み、その処理を模倣して解決しました。
そのときのプログラムがまた、結構すごいものだったので、
機密保持契約及び、著作権侵害にならない程度に紹介したいと思います。
1. グローバル変数が多い
プロセス名なんかはプログラム内で不変なのでよくグローバル変数で定義しますが、
ソケットまでグローバルだとあとから読む人は大変です。
数えたら、2500行程度の小規模なプログラムで、30個はグローバル変数がありました。
2. 文字列の扱いが苦手
バッファに出力するためのメッセージ整形を strcat() だけに頼っていました。
実際この様な感じです。
strcpy(buf, id);
strcat(buf, " ");
strcat(buf, func_id);
strcat(buf, " NG" );
本来ならば sprintf(buf, "%s %s NG", id, func_id); と一行で書けますよね。
3. いろいろ間違っている
下記の様な関数もありました。※もちろん下記関数は私が本稿のために書き直したものです。そのまま載せるのはマズいので・・・
static char *
blank_to_zero(char *str)
{
static char buf[10];
char *p = buf;
while (*str != 0) {
if (*str == ' ') {
*p = '0';
} else {
*p = *str;
}
p++;
str++;
}
*p = '\0';
return (buf);
}
どうやらスペース文字だけを数字の 0 に置換する関数の様です。
が、突っ込みどころが満載です。
まず戻り値に static な char型配列へのポインタを使っています。
間違いではありませんが、下記の様な使いかたをしたときに問題がおきます。
char buf_1 = " A";
char buf_2 = " B";
char *p_1, *p_2;
p_1 = blank_to_zero(buf_1);
p_2 = blank_to_zero(buf_2);
printf("%s\n", p_1);
printf("%s\n", p_2);
このプログラムから期待する出力結果は通常、
「
00A
00B
」
だと思いますが、上記関数を使うと、
「
00B
00B
」
になってしまいます。
それは、static 宣言された buf がプロセス中唯一であることに起因します。
よって、マルチスレッドで利用した場合はもっと酷く、でたらめな文字列になってしまうこともあるでしょう。
マルチスレッドの場合もスタックは独立していることを利用して、
buf を非static 宣言にして、return (strdup(buf)); とすればまだ被害が無く良いですね。
＃free() 忘れには気をつけなくてはいけませんが・・・
次に、
while (*str != 0) {
という書き方が問題です。
本来は *str != '\0' ですね。
多くの処理系実装では '\0'(ナル文字) を 0 と扱っても問題なくプログラムは動作しますが、
意味が全く違います。
'\0' は文字列の終端、0 は数値としてのゼロです。
それに関数の終り付近で、
p++;
str++;
としているので、while() ではなく for() ループでも良い気がします。
また buf[10] なのに、与文字長をチェックするコードが無い点も気になりますね。
ちょっと長い文字列を与えれば簡単にバッファオーバフローを起してしまいます。
長い紹介になりましたが、如何でしょうか。
世に出回って皆さんの生活を担っているシステムにもこのような大変なことになっているコードが沢山ありそうです。
そういえば、20日には WIDE 20周年のイベントがありました。
社長が参加してお土産を持ってきてくれました。
＃私は参加できませんでした；
これまた飾っておくと大変なことになりそうな M&Mチョコレートでした(笑)
