C
1
hamichamp@hamichamp
RevisionsEdit RequestsShow all likersShow article in Markdown
Report article

侍エンジニア塾のC言語のサンプルがヤバすぎる。

C

https://www.sejuku.net/blog/25002

C言語はもうかれこれ10年くらい書いていないけど、流石にこれはヤバい。

正直な感想として、ブランド毀損するくらいの危険性をはらんでいると思う。

当該記事からコピーしてきた。

#include <stdio.h>
#include <stdlib.h>

// 構造体の宣言
typedef struct {
    int num;
    char *str;
} strct;

int main(void) {
    // 実体を生成
    strct *entity;

    // 動的メモリの確保。確保したメモリをstrct型ポインタにキャスト。
    entity = (strct*)malloc(sizeof(strct));

    // メンバの初期化
    entity->num = 0;
    entity->str = (char*)malloc(sizeof(32));

    // メモリに文字列を代入
    sprintf(entity->str, "%s %s!", "Hello", "World");
    printf("%s\n", entity->str);

    // メモリの解放
    free(entity->str);
    free(entity);

    return 0;
}

まず気になるのが、このコメント

    // 実体を生成
    strct *entity;

実体というのは、どういう意味で書いたんだろう。
これはただのポインタなんだけど・・・。

個人的には、変数の宣言と同時に初期化するのが好きなので、私ならこう書くと思う。

    strct *entity = (strct　*)malloc(sizeof(strct));

この辺は、まだいい。
驚愕のコードがここだ。

    entity->str = (char*)malloc(sizeof(32));

sizeof(32)が何を返すか理解していないと思う。
32はint型だ。
これはsizeof(int)と同じで、処理系にもよるけど、大抵のパターンは4が返ってくるんじゃないかと思う。
つまり、4バイト分のメモリしか確保していないんですけど、大丈夫？（大丈夫じゃない）

仮に32文字分のメモリを確保したいとしたら、

    entity->str = (char *)malloc(sizeof(char) * 32);

と書けば良いかと思う。
あと、メモリ確保に失敗することもあるので、戻り値のNULLチェックはしたほうがいい。いや、するべきだ。

    // メモリに文字列を代入
    sprintf(entity->str, "%s %s!", "Hello", "World");

怖くて、正直こんなコードはなかなか本番じゃ書かないと思うけど、バッファオーバーフローの危険性をはらんでいる。
実際、初期のコードだとバッファオーバーフローしている。

このコードについては、これくらいにして、次のコード。

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

// 構造体の宣言
typedef struct {
    int num;
    char *str;
} strct;

int main(void) {
    // 実体を生成
    strct *entity;

    // 動的メモリの確保。確保したメモリをstrct型ポインタにキャスト。
    entity = (strct*)malloc(sizeof(strct));

    // メンバの初期化
    entity->num = 0;
    entity->str = (char*)malloc(sizeof(32));

    // メモリに文字列を代入
    sprintf(entity->str, "%s %s!", "Hello", "World");
    printf("%s\n", entity->str);

    char arr_str[] = "Hello USA!";

    // メモリサイズの変更
    entity->str = (char*)malloc(sizeof(arr_str));
    if(entity->str == NULL) {
        printf("memory error");
        return -1;
    }

    // アドレスの先頭からarr_strのバイト数分だけNULLで書き換え
    memset(entity->str, '\0', sizeof(arr_str));

    printf("%s\n", entity->str);

    // メモリの解放
    free(entity->str);
    free(entity);

    printf("processing completion\n");

    return 0;
}

sizeof(32)はさっき指摘したので省略するとして、

    entity->str = (char*)malloc(sizeof(arr_str));

おーい、これもともとあったentity->strのメモリはどうするんだよ。
立派なメモリリークの誕生だよ。

次！

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

// 構造体の宣言
typedef struct {
    int num;
    char *str;
} strct;

int main(void) {
    // 実体を生成
    strct *entity;

    // 動的メモリの確保。確保したメモリをstrct型ポインタにキャスト。
    entity = (strct*)malloc(sizeof(strct));

    // メンバの初期化
    entity->num = 0;
    entity->str = (char*)malloc(sizeof(32));

    // メモリに文字列を代入
    sprintf(entity->str, "%s %s!", "Hello", "World");
    printf("%s\n", entity->str);

    //構造体の実体のコピー
    strct *copy_entity;
    copy_entity = (strct*)malloc(sizeof(strct));
    memcpy(copy_entity, entity, sizeof(strct)); // メンバのポインタは浅いコピー

    // 浅いコピーのため、コピー元の値が変わるとコピー先の値も変わる
    strcpy(entity->str, "Hello Japan!");
    printf("%s, %s\n", entity->str, copy_entity->str);

    // 深いコピーにするためには、メンバ個別でコピーが必要
    copy_entity->str = (char*)malloc(sizeof(32));
    strcpy(copy_entity->str, entity->str);

    // 深いコピーのため、コピー元の値が変わってもコピー先の値は変わらない
    strcpy(entity->str, "Hello USA!");
    printf("%s, %s\n", entity->str, copy_entity->str);

    // メモリの解放
    free(copy_entity->str);
    free(copy_entity);
    free(entity->str);
    free(entity);

    return 0;
}

sizeof(32)は・・・（省略）

    memcpy(copy_entity, entity, sizeof(strct)); // メンバのポインタは浅いコピー

悪いとは言わないけど、このパターンなら、*copy_entity = *entityって私なら書くかな。
memcpyのサンプルにしたいなら、構造体の配列くらい用意してもいいかもね。

最後！

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

// 構造体の宣言
typedef struct {
    int num;
    char *str;
} strct;

int main(void) {
    // 実体を生成
    strct *entity;

    // 動的メモリの確保。確保したメモリをstrct型ポインタにキャスト。
    entity = (strct*)malloc(sizeof(strct));

    // メンバの初期化
    entity->num = 0;
    entity->str = (char*)malloc(sizeof(32));

    // メモリに文字列を代入
    sprintf(entity->str, "%s %s!", "Hello", "World");
    printf("%s\n", entity->str);

    //構造体の実体のコピー
    strct *copy_entity;
    copy_entity = (strct*)malloc(sizeof(strct));
    memcpy(copy_entity, entity, sizeof(strct)); // メンバのポインタは浅いコピー

    // コピー元とコピー先を比較演算
    if( memcmp(entity, copy_entity, sizeof(&copy_entity)) == 0) {
        printf("構造体の実体%sと%sは同じです\n", "entity", "copy_entity");
    } else {
        printf("構造体の実体%sと%sは別です\n", "entity", "copy_entity");
    }

    // 深いコピーにするためには、メンバ単体でコピーが必要
    copy_entity->str = (char*)malloc(sizeof(32));
    strcpy(copy_entity->str, entity->str);

    // コピー元とコピー先を比較演算
    if(memcmp(entity, copy_entity, sizeof(&copy_entity)) == 0) {
        printf("構造体の実体%sと%sは同じです\n", "entity", "copy_entity");
    } else {
        printf("構造体の実体%sと%sは別です\n", "entity", "copy_entity");
    }

    // メモリの解放
    free(copy_entity->str);
    free(copy_entity);
    free(entity->str);
    free(entity);

    return 0;
}

このコード大丈夫？（だから、大丈夫じゃないって）

    // コピー元とコピー先を比較演算
    if( memcmp(entity, copy_entity, sizeof(&copy_entity)) == 0) {
        printf("構造体の実体%sと%sは同じです\n", "entity", "copy_entity");
    } else {
        printf("構造体の実体%sと%sは別です\n", "entity", "copy_entity");
    }

特に、sizeof(&copy_entity)この意味理解してる？
copy_entityの型ってstrct *でしょ？それに&をつけると、どうなるの？

はい！strct **型を表します。

で、そのsizeofってことは？？？

・・・

処理系にもよるけど、大抵の場合、4か８が返ってくると思うよ。
つまり、構造体のint numに相当する部分しか比較していないと思うんだけど・・・。
もし書き直すとしたら、sizeof(strct)かな。
間違ってもsizeof(copy_entity)じゃないからね！

ついでに言うと、パディング云々言ってるわけだから、構造体もメモリ確保した後、memset使って初期化したほうがいいんじゃないかな？

正直なところmemcmpで比較するんじゃなくて、比較用の関数を作ってメンバの中身までをチェックしたほうがいいと思う。C++なら=演算子のオーバーロードだね。

最後に・・・
間違いは誰にでもある。
コード憎んで、人を憎まず。
技術系の記事って検証に検証を重ねないと怖いですね。

Edit request
23
Sign in to continue the action.
Sign in with EmailSign in with TwitterSign in with GitHubSign in with Google
or