mallocで色々メモリ確保したので全部freeしたい

Question

いつもお世話になっております。
初心者なので、初歩的な質問ばかりで申し訳ありません。
以下のコードの通り、Aという疎行列とその転置であるBを作成し、それらをsmatrix_free(A)やsmatrix_free(B)でfreeしたいです。

そこでコンパイル後にvalgrind --leak-check=full ./a.outによりメモリを全部解放できているかどうか調べてみたところ、残念ながら解放できていないようです。

単純な書き間違いなのか、根本的にコードがおかしいのかよく分かりません。
どなたか、ご教授いただければ幸いです。
宜しくお願いいたします。

編集：コードが長ったらしい上に、実際valgrindでどういう出力があったのかについては全く言及しておりませんでしたね。大変失礼いたしました。

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

#define NEW(p, n){p = malloc((n)*sizeof(p[0]));} //動的メモリ確保の関数

//リストの要素の構造体
typedef struct slobj_{
    struct slobj_* next;
    int j;
    double v;
}* slobj;

//リストの構造体
typedef struct{
    slobj head;
    slobj tail;
}* slist;

//疎行列の構造体(リストの配列ということ)
typedef struct{
    int n,m;
    slist* A;
} smatrix;　

//新しいリストを作成
slist slist_new(void){
    slist L;
    NEW(L, 1);
    L -> head = NULL;
    L -> tail = NULL;
    return L;
}

//新しいリストの要素を作成
slobj slobj_new(int x, double y){
    slobj p;
    NEW(p, 1);
    p -> j = x;
    p -> v = y;
    p -> next = NULL;
    return p;
}

//リストの先頭に挿入
void slist_insert_head(slist L, slobj p){
    p -> next = L -> head;
    L -> head = p;
}

//リストの末尾に挿入
void slist_insert_tail(slist L, slobj p){
    if(L -> head == NULL){
        L -> tail = p;
        L -> head = p;
    }
    else if(L -> head == L -> tail){
        L -> tail = p;
        L -> head -> next = p;
    }
    else{
        L -> tail -> next = p;
        L -> tail = p;
    }
}

//リストをプリント
void slist_print(slist L)
{
    slobj p;
    p = L->head;
    while (p != NULL){
        printf("%d ", p -> j);
        printf("%lf ", p -> v);
        p = p-> next;
    }
}

//新しい疎行列を作成
smatrix smatrix_new(int n, int m){
    smatrix S;
    int i;
    NEW(S.A, n);
    S.n = n;
    S.m = m;
    for(i=0; i<= n-1; i++){
        S.A[i]=slist_new();
    }
    return S;
}

//標準出力から行列を読み込み、別のメモリに記憶しておく
smatrix smatrix_read(void){
    smatrix S;
    int i, n, m, j;
    double v;
    scanf("%d", &n);
    scanf("%d", &m);
    S = smatrix_new(n, m);
    for(i = 0; i <= n-1; i++){
        while(1){
            scanf("%d", &j);
            if (j < 0){
                break;
            }
            scanf("%lf", &v);
            slist_insert_tail(S.A[i], slobj_new(j ,v));
//このときslobj_newで確保したメモリがslist_freeで解放できていないようです
        }
    }
    return S;
}

//疎行列をプリント
void smatrix_print(smatrix S){
    int i;
    printf("%d ", S.n);
    printf("%d\n", S.m);
    for(i = 0; i <= S.n-1; i++){
        slist_print(S.A[i]);
        printf("%d\n", -1);
    }
}

//疎行列の転置を作成・出力
smatrix smatrix_transpose(smatrix S){
    int i, k;
    double x;
    slobj p;
    smatrix T;
    T = smatrix_new(S.m, S.n);
    for(i = 0; i <= S.n-1; i++){
        p = S.A[i]->head;
        while(1){
           if(p==NULL){
               break;
           }
           k = p->j;
           x = p->v;
           slist_insert_tail(T.A[k-1], slobj_new(i+1, x));
//このときslobj_newで確保したメモリがslist_freeで解放できていないようです
           p = p->next;
        }
    }
    return T;   
}

//リストのメモリを解放
void slist_free(slist L){
    slobj p, q, r;
    p = L->head;
    q = p->next;
    while (q != NULL){
        r = q;
        q = q-> next;
        free(p);
        p = r;
    }
    free(L);
}

//疎行列のメモリを解放(リストを順にfreeしているつもりです)
void smatrix_free(smatrix S){
    int i;
    for(i = 0; i <= S.n-1; i++){
            slist_free(S.A[i]);
    }
    free(S.A);
}

int main(){
    smatrix A;
    smatrix B;
    A = smatrix_read();
    B = smatrix_transpose(A);
    smatrix_print(B);
    smatrix_free(A);
    smatrix_free(B);    
    return 0;
}

valgrindでの出力は以下の通りでした。

valgrind --leak-check=full ./a.out
==152== Memcheck, a memory error detector
==152== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==152== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==152== Command: ./a.out
==152==
==152== error calling PR_SET_PTRACER, vgdb might block
2 3
1 1.0 2 2.0 3 3.0 -1
1 4.0 2 5.0 3 6.0 -1
3 2
1 1.000000 2 4.000000 -1
1 2.000000 2 5.000000 -1
1 3.000000 2 6.000000 -1
==152==
==152== HEAP SUMMARY:
==152==     in use at exit: 120 bytes in 5 blocks
==152==   total heap usage: 21 allocs, 16 frees, 8,600 bytes allocated
==152==
==152== 48 bytes in 2 blocks are definitely lost in loss record 1 of 2
==152==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==152==    by 0x1087F6: slobj_new (42.c:33)
==152==    by 0x108A5A: smatrix_read (42.c:97)
==152==    by 0x108CED: main (42.c:158)
==152==
==152== 72 bytes in 3 blocks are definitely lost in loss record 2 of 2
==152==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==152==    by 0x1087F6: slobj_new (42.c:33)
==152==    by 0x108BD2: smatrix_transpose (42.c:127)
==152==    by 0x108D08: main (42.c:159)
==152==
==152== LEAK SUMMARY:
==152==    definitely lost: 120 bytes in 5 blocks
==152==    indirectly lost: 0 bytes in 0 blocks
==152==      possibly lost: 0 bytes in 0 blocks
==152==    still reachable: 0 bytes in 0 blocks
==152==         suppressed: 0 bytes in 0 blocks
==152==
==152== For counts of detected and suppressed errors, rerun with: -v
==152== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

このように、smatrix_readやsmatrix_transposesで新しい行列を作成し、そこに順次リストの要素を挿入していく際に確保したリスト要素のメモリはsmatrix_freeやその中のslist_freeで全て解放しているはずなのですが、どうやら全部解放しきれていないようです。(問題がありそうな個所はコードの中でその旨をコメントしておきました)

slist_freeの記述がおかしいのでしょうか。

Comments

[jimbe]

> 単純な書き間違いなのか、根本的にコードがおかしいのか

何処までご自分で調査されたのでしょうか.

[Zuishin]

プロセスが終了すると全て解放されるので別に問題ありませんが、new が散らばりすぎて見通し悪いのが原因です。

[Zuishin]

もう一度書きますが、プロセスが終了すると全て解放されるので別に問題ありませんが、new が散らばりすぎて見通し悪いのが原因です。

とりあえず今、さしあたって、場当たり的に、泥縄的に、直すのもいいですが、見通しが悪い(長いというのはオブラートに包んだ表現です)のが諸悪の根源なので、それをどうにかしましょう。

[Zuishin]

つまり、設計し直して、最初から最後まで総書き換えです。

[Zuishin]

あと、テストを作りましょう。

[jimbe]

> slist_freeの記述がおかしいのでしょうか。
そう思われましたら, Zuishin さんの言われるように 0件/1件/2件(以上)の3パターンのリストが渡された場合にそれぞれどのように slist_free が動作するか(リスト内の全ての slobj に対し free を実行出来ているか) を, 別の main を作ってテストしてみては如何でしょうか.
(ポインタに慣れていれば思考実験も出来るかと思いますが.)

Answer 1

根本的に

ということで, マトリクス生成時に大きさが決まっているのでリストにする必要が無いようですし, 実体とポインタが混在して分かり難く感じましたので, リスト関係を消し, SMATRIX の malloc をするようにしてみました.
大きな行列で無ければこのほうが解放抜けはし難いと思います.
テストを簡単にするため, smatrix_read を改造しています.

#修正
リストを使うように戻し, 2次元的に動作するようにしました.
データが無い所を 0 表示するようにしました.

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

//要素の構造体
typedef struct slobj {
  struct slobj *next; //必ず構造体の先頭にすること
  int x, y;
  double v;
} SLOBJ;

//新しい要素を作成
SLOBJ *slobj_new(int x, int y, double v){
  SLOBJ *p;

  p = (SLOBJ *)malloc(sizeof(SLOBJ));
  p->next = NULL;
  p->x = x;
  p->y = y;
  p->v = v;
  return p;
}

//リストの構造体
typedef struct slist {
  SLOBJ *next; //必ず構造体の先頭にすること
  SLOBJ *last;
} SLIST;

//初期化
void slist_init(SLIST *slist) {
  slist->next = NULL;
  slist->last = (SLOBJ *)slist;
}

//書き換えor追加
void slist_put(SLIST *slist, int x, int y, double v) {
  SLOBJ *p;

  for(p=slist->next; p!=NULL; p=p->next) {
    if(p->x == x && p->y == y) {
      p->v = v;
      return;
    }
  }
  p = slobj_new(x, y, v);
  slist->last->next = p;
  slist->last = p;
}

//取り出し
double slist_get(SLIST *slist, int x, int y) {
  SLOBJ *p;

  for(p=slist->next; p!=NULL; p=p->next) {
    if(p->x == x && p->y == y) return p->v;
  }
  return 0.0;
}

//解放
void slist_clear(SLIST *slist) {
  SLOBJ *p, *q;
  for(p=slist->next; p!=NULL; p=q) {
    q = p->next;
    free(p);
  }
  slist_init(slist);
}

//疎行列の構造体
typedef struct {
  int n, m;
  SLIST a;
} SMATRIX;

//新しい疎行列を作成
SMATRIX *smatrix_new(int n, int m) {
  SMATRIX *s;

  s = (SMATRIX *)malloc(sizeof(SMATRIX));
  s->n = n;
  s->m = m;
  slist_init(&s->a);
  return s;
}

//標準出力から行列を読み込み、別のメモリに記憶しておく
SMATRIX *smatrix_read() {
  int i, n, m, x, y, k=0;
  double v;
  SMATRIX *s;
  int test[] = { 2,3,  1,1,1, /*2,1,2*/ 1,2,4,  2,2,5, 3,2,6, 3,1,3, }; //テストデータ

  n = test[k++]; //scanf("%d",&n);
  m = test[k++]; //scanf("%d",&m);
  s = smatrix_new(n, m);
  for (i=0; i<(sizeof(test)/sizeof(int)-2)/3; i++) {
    x = test[k++]; //scanf("%d",&s->a[i].j);
    y = test[k++];
    v = test[k++]; //scanf("%lf",&s->a[i].v);
    slist_put(&s->a, x, y, v);
  }
  return s;
}

//疎行列の転置を作成・出力
SMATRIX *smatrix_transpose(SMATRIX *s) {
  SLOBJ *p;
  SMATRIX *t;

  t = smatrix_new(s->m, s->n);
  for(p=s->a.next; p!=NULL; p=p->next) {
    slist_put(&t->a, p->y, p->x, p->v); //行列変換
  }
  return t;
}

//疎行列をプリント
void smatrix_print(char *label, SMATRIX *s) {
  if(label != NULL) printf("%s\n", label);
  printf("%d %d\n", s->n, s->m);

  for(int y=1; y<=s->n; y++) {
    for(int x=1; x<=s->m; x++) {
      printf("(%d,%d)=%lf%s", x, y, slist_get(&s->a,x,y), x==s->m?"\n":" ");
    }
  }
}

void smatrix_free(SMATRIX *s) {
  slist_clear(&s->a);
  free(s);
}

int main() {
  SMATRIX *a;
  SMATRIX *b;

  a = smatrix_read();
  b = smatrix_transpose(a);
  smatrix_print("A:", a);
  smatrix_print("B:", b);
  smatrix_free(a);
  smatrix_free(b);
  return 0;
}

A:
2 3
(1,1)=1.000000 (2,1)=0.000000 (3,1)=3.000000
(1,2)=4.000000 (2,2)=5.000000 (3,2)=6.000000
B:
3 2
(1,1)=1.000000 (2,1)=4.000000
(1,2)=0.000000 (2,2)=5.000000
(1,3)=3.000000 (2,3)=6.000000

Comments

[aardvark]

ありがとうございます。
勉強になりました。

Answer 2

slist_freeで最後の要素をメモリ解放していなかっただけでした。
お騒がせしました。

Answer 3

コードは長いので全く読んでませんが、「不要になったらすぐにfreeする」という方針にしていますか？
それでも漏れるとしたら、「不要になった」という判断が間違っているので、その観点でポインタをつなぎ替える処理を全部見なおしでしょうか。

Answer 4

https://www.geeksforgeeks.org/dynamic-memory-allocation-in-c-using-malloc-calloc-free-and-realloc/

Answer 5

https://www.geeksforgeeks.org/dynamic-memory-allocation-in-c-using-malloc-calloc-free-and-realloc/