C++：動的確保時のメモリリークとValgrindについて

Question

初心者の学生です。どうぞ宜しくお願いいたします。

動的確保の課題を解いています。
CSVファイルを読み込んで、GPA（成績データ）でソートして表示する課題です。

送信サーバ（Linux）上の Valgrind というメモリリークをチェックするプログラムにより
課題の送信が阻まれております...
C言語のクラスからC++のクラスに進級した途端、これでチェックされるようになり毎回困っています...

お知恵をお借りできれば幸いです。お時間ありがとうございます。

Valgrindによるエラー画面

画面内のエラーメッセージをGoogle翻訳してみましたが、全く意味がわかりません...

• 条件付きジャンプまたは移動は、初期化されていない値によって異なります
• エラー：読み取られたレコードの数が正しくありません。 データが破損している可能性があります

また、Linuxサーバ上でg++してみたところ、

gpaReport.cpp:(.text+0x5): undefined reference to `sdds::load()'
gpaReport.cpp:(.text+0xe): undefined reference to `sdds::display()'
gpaReport.cpp:(.text+0x13): undefined reference to `sdds::deallocateMemory()'

と出ました。何か手がかりになりますでしょうか...

ソースコード

#define _CRT_SECURE_NO_WARNINGS
#define DATAFILE "students.csv"
#include <iostream>
#include <cstring>
#include <cstdio>
using namespace std;

namespace sdds {
	struct Student {
		char* m_name;
		int m_studentNumber;
		double m_gpa;
	};

	FILE* fptr;
	int noOfStudents;
	Student* students;

	// 学生のGPA順に動的配列を並べ替え
	void sort();
	// ファイルから値を含む学生構造をロード
	bool load(Student& record);
	// 学生の動的配列を割り当て、すべてのファイルレコードを配列にロード
	bool load();
	// 学生レコードを表示 (for gpaReport.cpp)
    void display();
    // 学生レコードを表示
    void displayStudentRecord();
	// 最初に生徒をソートしてから、すべての生徒を表示
	void displayAllStudents();
 	// 配列の割り当てを解除
	void deallocateMemory();
}

namespace sdds {
	bool openFile(const char filename[]);
	void closeFile();
	int noOfRecords();
	bool readName(char name[]);
	bool readNumber(int *number);
	bool readGpa(double *gpa);
}

namespace sdds {
	void sort() {
		int i, j;
		Student temp;
		for (i = noOfStudents - 1; i > 0; i--) {
			for (j = 0; j < i; j++) {
				if (students[j].m_gpa > students[j + 1].m_gpa) {
					temp = students[j];
					students[j] = students[j + 1];
					students[j + 1] = temp;
				}
			}
		}
	}

	// ファイルから1つの学生レコードを読み取り、学生参照引数にロード
	bool load(Student& record) {
		bool ok = false;
		char name[128];
		// 名前の読み取りが成功した場合
		if (readName(name)) {
			ok = readNumber(&record.m_studentNumber) && readGpa(&record.m_gpa);
			if (ok) {
				int nameSize = 0;
				nameSize = strlen(name) + 1;        // 名前のサイズ+1にメモリを割り当てます
				record.m_name = new char[nameSize]; // アドレスを参照の名前で保持
				strcpy(record.m_name, name );       // 名前を新しく割り当てられたメモリにコピー
				ok = true;                          // 問題なければ okフラグをtrueに設定
			}
		}
		return ok;
	}

	// the 0 arg load function 
	bool load() {
		bool ok = false;
		int i = 0;
		if (openFile(DATAFILE)) {
            noOfStudents = noOfRecords(); // ファイル内のレコード数に設定

			// 学生の配列をグローバルに動的に割り当て
			students = new Student[noOfStudents];

			for (i = 0; i < noOfStudents; i++) {// ループ内で学生レコードをファイルから動的配列にロード
				bool res = load(students[i]);
				if (res == false) {
				    break;
				}
			}
			// レコードの数が読み取り数と一致しない場合、エラーメッセージを出力
			if (i != noOfStudents){
                cout << "Error: incorrect number of records read; the data is possibly corrupted" << endl;
			}
			else {
				ok = true;
			}
			closeFile();
		}
		else {
			cout << "Could not open data file: " << DATAFILE<< endl;
		}
	return ok;
	}

	// 学生レコードを表示 (for gpaReport.cpp)
	void display() {
	  displayAllStudents();
	}

	// 学生レコードを表示
	void displayStudentRecord(const Student *record) {
		cout << record->m_name << ", "
			<< record->m_studentNumber << ": "
			<< record->m_gpa << endl;
	}

	// 最初に生徒をソートしてから、すべての生徒を表示
	void displayAllStudents() {
		int i;
		sort();
		for (i = 0; i < noOfStudents; i++) {
		    cout << i+1 << ": ";
		    displayStudentRecord(&students[i]);
		}
	}

	// 最初にstudent要素のすべての名前の割り当てを解除し、次にstudent配列の割り当てを解除
	void deallocateMemory() {
		if (noOfStudents <= 0 || students == NULL) {
		    return;
		}
		// ループしてstudents配列のすべての要素を調べ、各学生の動的名の割り当てを解除
		for (int i = 0; i < noOfStudents; i++) {
		    delete [] students[i].m_name;
		    students[i].m_name = nullptr;
		}
		delete [] students; // students配列全体の割り当てを解除
		students = nullptr;
	}
}

namespace sdds {
	bool openFile(const char filename[]) {
		fptr = fopen(filename, "r");
		return fptr != NULL;
	}
	int noOfRecords() {
		int noOfRecs = 0;
		char ch;
		while (fscanf(fptr, "%c", &ch) == 1) {
			noOfRecs += (ch == '\n');
		}
		rewind(fptr);
		return noOfRecs + 1; // 最終行用に +1
	}
	void closeFile() {
		if (fptr) fclose(fptr);
	}

	bool readName(char name[]) { // データファイル内の学生名の読み取り
		char names[256];
		int gi = 0; //names index;
		int i = 0;// name[i] index
		bool res = fscanf(fptr, "%[^,],", names);
		if (res) { //csvを文字列の配列に拡散する
			res = false;
			while (names[gi]) {
				name[i++] = names[gi++];
				res = true;
			}
		}
		name[i++] = '\0';
		name[i] = '\0'; // 文字列の配列を終了する
		return res;
	}

	bool readNumber(int *number) { // データファイル内の学生番号の読み取り
	   return fscanf(fptr, "%d,", number) == 1;
	}

	bool readGpa(double *gpa) { // データファイル内のGPAの読み取り
	   return fscanf (fptr, "%lf\n", gpa) == 1;
	}

}

using namespace sdds;

int main() {
    if (load()) {
        display();
    }
    deallocateMemory();
    return 0;
}

データファイル

Abraham Simpson,324543,3.9
Alice Glick,459608,1.9
Allison Taylor,747954,3.2
Apu Nahasapeemapetilon,290816,2.6
Bart Simpson,753102,3.9
Bernice Hibbert,242653,3.8
Carl Carlson,241968,3.7
Homer Simpson,413084,2.9
Lisa Simpson,693664,4.0
Luann Van Houten,737447,2.3
Martin Prince,575687,3.5
Maude Flanders,272754,1.7
Sarah Wiggum,920562,2.0
Surly Duff,146127,2.6

試したこと

• new の格納先を +1 してみた --> 何も変わりませんでした（追加前と同じエラー）
• delete を追加してみた --> 余計にエラーが増えました...
• コマンドで'valgrind result' と打っても 'command not found' と言われてしまい、行番号すらわかりませんでした...

ご回答をいただき追加で試したこと

• delete [] とした（投稿時のスクショは [] ありのもので、投稿したコードが古かったです、すみません）
• students[i].m_name と students に nullptr を追加

上記を試してみましたが、Valgrindからは同じ出力が出ます...

Comments

[SHOMI]

> students[i].m_name と students に nullptr を追加
なぜ解放後に…

Answer 1

delete students[i].m_name;
delete students; // students配列全体の割り当てを解除

delete[]じゃないの？

---

        for (int i = 0; i < noOfStudents; i++) {
            delete students[i].m_name;
        }

レコード数を読めたけれど、名前を読めなかった場合それ以降の要素のm_nameは不定値なのでdeleteしてはまずいのでは？
Studentにコンストラクタをもたせるか、students = new Student[noOfStudents];の後で全要素nullptrで初期化するべき。

---

画面内のエラーメッセージをGoogle翻訳してみましたが、全く意味がわかりません...

エラー：読み取られたレコードの数が正しくありません。 データが破損している可能性があります

あなたの書いたプログラムが出しているメッセージですが…

            // レコードの数が読み取り数と一致しない場合、エラーメッセージを出力
            if (i != noOfStudents){
                cout << "Error: incorrect number of records read; the data is possibly corrupted" << endl;
            }

---

Conditional jump or move depends on uninitialised value(s)

ファイルを正しく読めておらず未初期化の変数namesをwhileの条件に使用して怒られています。
以下を直せば今指摘されているValgrindのエラーは消えます。

    struct Student {
        char* m_name;
        int m_studentNumber;
        double m_gpa;
+        Student() : m_name( nullptr){}
    };

-    Student* students;
+    Student* students = nullptr;

    bool readName(char name[]) { // データファイル内の学生名の読み取り
        char names[256];
        int gi = 0; //names index;
        int i = 0;// name[i] index
-        bool res = fscanf(fptr, "%[^,],", names);
+        bool res = 1 == fscanf(fptr, "%[^,],", names);
        if (res) { //csvを文字列の配列に拡散する
            res = false;
            while (names[gi]) {
                name[i++] = names[gi++];
                res = true;
            }
        }
        name[i++] = '\0';
        name[i] = '\0'; // 文字列の配列を終了する
        return res;
    }

---

students.csvが14行にも関わらずnoOfRecords()では15が返り、
15行目を読もうとして上記ValgrindのメッセージとError: incorrect number of records read; the data is possibly corruptedが出力されているのでしょう。

    int noOfRecords() {
        int noOfRecs = 0;
        char ch;
        while (fscanf(fptr, "%c", &ch) == 1) {
            noOfRecs += (ch == '\n');
        }
        rewind(fptr);
-        return noOfRecs + 1; // 最終行用に +1
+        return noOfRecs;
    }

Comments

[aluminium]

ありがとうございます。
new した所、全部に delete を入れてみましたが、余計にエラーが出てしまったんです...

[SHOMI]

>new した所、全部に delete を入れてみました
どこにどう入れたんですか？
コードを理解した上で入れましょう

[cateye]

deleteとdelete[]は違いますよ。→aluminiumさん

[aluminium]

> SHOMI 様
わかりづらく、申し訳ありませんでした。

85行目の students = new Student[noOfStudents];
69行目の record.m_name = new char[nameSize];

に対応するように入れてみましたが、結果は同じでした。

nullptr も 追加してみましたが、結果は同じでした。

コンストラクタ とは何か調べてみたのですが、よくわからず...
どう実装したらいいでしょうか？

[aluminium]

> cateye 様
ありがとうございます。

[aluminium]

仰るとおり、return noOfRecs; に +1 してしまっていました... これが原因でした！！
別のコンパイラで動かしてみたとき、最終行が上手く出力されず、
安易に +1 してしまったのがいけなかったです。

SHOMIさんのコメント「おそらくstudents.csvが14行にも関わらずnoOfRecords()では15が返り」
本当に助かりました！また、色々試してアドバイスをいただき、ありがとうございました！!

最初から課題送信と同じサーバ上でコンパイルするべきですね... 勉強になりました。