C言語のプログラムを書いたのですが上手く動きません

質問

C言語の練習問題をプログラミングしたのですが、上手く動きません。
コンパイルはできるのですが、実行すると「Segmentation fault(core dumped)」となります。

問題は、コマンドライン引数としてファイル名を指定したテキストファイルから読み込んだデータを，双方向リストに格納し，順番に表示するというものです。
テキストファイルは

10T5001 C
10T5002 A
10T5003 B
10T5004 C
10T5005 D
10T5006 B
10T5007 A
10T5008 D

このように、IDとランクをランクABCDをスペースで区切ったもので、これをランクAから順番に表示させます。
上手く動けば

ID: 10T5002, grade: A,
ID: 10T5007, grade: A
ID: 10T5003, grade: B
ID: 10T5006, grade: B
ID: 10T5001, grade: C
ID: 10T5004, grade: C
ID: 10T5005, grade: D
ID: 10T5008, grade: D

と表示されるはずなんですが・・・

これが僕の書いたプログラムです（長いです。ごめんなさい）

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

#define ID_LENGTH 8 /* IDの長さ+ナル文字 */
#define EFOPEN -1
#define ENOMEM -2
#define EINVAL -3

/* 双方向リストのノード */
typedef struct sList /* タグ */
{
char id[ID_LENGTH]; /* ID */
char grade; /* ランク*/
struct sList *prev; /* 前のノードのアドレス */
struct sList *next; /* 次のノードのアドレス */
} sNode;

/*
双方向リストのノードを作成する関数 makeNewNode()
作成したノードのprevとnextはNULLにする
引数
・ノードに格納するID
・ノードに格納するランク(A/B/C/D)
戻値
作成したノードの先頭アドレス．メモリの確保に失敗した場合はNULL
*/

sNode *makeNewNode(char *id, char grade)
{
sNode *pNewNode;
pNewNode = (sNode*)malloc(sizeof(sNode));

if(pNewNode != NULL)
{
strncpy(pNewNode->id, id, ID_LENGTH-1);
pNewNode->grade = grade;
pNewNode->prev = NULL;
pNewNode->next = NULL;
}

return pNewNode;
}

/*
引数で渡された任意のノードの後ろに新しいノードを追加する関数 insertNext()
引数
・後ろにノードを追加したいノードの先頭アドレス
・追加するノードの先頭アドレス
戻値 なし
*/

void insertNext(sNode *node, sNode *newNode)
{
if(node->next == NULL) //一番後ろに追加する場合
{
node->next = newNode;
newNode->prev = node;
}
else //まん中に挿入する場合
{
newNode->prev = node;
newNode->next = node->next;
node->next->prev = newNode;
node->next = newNode;
}
}

/*
main()
引数
int argc コマンドライン引数の数
char *argv[] 与えられたコマンドライン引数の文字列の先頭アドレスの配列
戻値
int 正常終了の時 0
以下の場合は異常終了し，括弧内の値を返す
データファイルが開けなかった場合(EFOPEN)
makeNewNode()でメモリが確保できなかった場合(ENOMEM)
引数の数が正しくない場合(EINVAL)
*/
int main(int argc, char *argv[]) {
sNode *top; /* リストの先頭ノードのアドレスを保持する変数*/
sNode *new; /* 新しく作成したノードのアドレスを保持する変数 */
sNode *now; /* 現在見ているノードのアドレスを保持する変数 */
FILE *fp; /* データファイルのファイルポインタ */
char id[ID_LENGTH]; /* ファイルから読み込んだIDを一時的に保持する変数 */
char grade; /* ファイルから読み込んだランクを一時的に保持する変数 */

/* コマンドライン引数の数をチェックする
数に過不足があれば，使い方を表示し，異常終了する */
if(argc != 2){
printf("Usage: %s datafilename.\n", argv[0]);
return EINVAL;
}

/* データファイルを読み込み用に開く
ファイルが開けなかった場合，エラーメッセージを表示し異常終了する */
fp = fopen(argv[1], "r");
if(NULL == fp){
printf("No such file %s.\n", argv[1]);
return EFOPEN;
}

/* リストの先頭に番兵を立てる */
new = makeNewNode("Banpei", 'A'-1);
if (new == NULL){
printf("Error: cannot allocate memory\n");
return ENOMEM;
}
top = new;

/* データファイルから1行ずつデータを読み込み，ランク順にリストに追加していく
既にリストの先頭には番兵ノードがある点に注意 */
while(EOF != fscanf(fp, "%s %c", id, &grade)){
new = makeNewNode(id, grade);

if(new == NULL){
printf("Cannot allocate memory.\n");
return EFOPEN;
}

if(top->next == NULL){ //リストが空の場合
insertNext(top, new);
}
else{
now = top;

while(new->grade > now->grade){
now = now->next;
}
insertNext(now, new);
}
}

/* できあがったリストの内容を先頭から順に表示する
ただし，番兵ノードは表示しない */
now = top;
while(now != NULL){
printf("ID: %s, grade: %c\n",now->id, now->grade,);
now = now->next;
}

/* リストのノードを全て(番兵ノードも含む)解放し，リストを空にする */
now = top;
while(now->next != NULL){
free(now->prev);
now = now->next;
}
free(now);

fclose(fp);

return (0);
}

まだまだ練習中なもので、かなり拙いものだと思いますが、とりあえずまずはプログラム動くようにしたいです。
よろしくお願いします。

回答No.2

gerugugu84

sNode *makeNewNode(char *id, char grade)
{
…
strncpy(pNewNode->id, id, ID_LENGTH-1);

　　→strncpy()の仕様では
　　　id <= ID_LENGTH-1の場合はヌル文字が付加されません。
　　　なので仮にソートがうまくいっても表示がおかしくなるはず。

　　　手っ取り早く直すならstrncpy()の直前で
　　　確保したノードエリアを初期化します。
　　　memset(pNewNode, 0, sizeof(sNode));
　　　strncpy(pNewNode->id, id, ID_LENGTH-1);

-----------------------------------------------------
int main(int argc, char *argv[]) {
…
while(new->grade > now->grade){
now = now->next;
}

　　→now->gradeがAで、new->gradeがBの場合、
　　　nowがnow->nextにズレますが、
　　　次のnow->nextがCの場合にwhile文を抜けても
　　　Cグレードの後ろに追加されてしまうので、
　　　順番がぐちゃぐちゃになります。

　　　ここではinsertNext(node, newNode)は
　　　nodeの後ろにnewNodeを挿入する仕様なので、
　　　おかしくなります。

　　　修正方法としては、２つ。
　　　１．insertNext(node, newNode)でnodeの前にnewNodeを挿入する
　　　２．上記while文を以下のように修正する
　　　　　for( now = top; NULL != now->next; now = now->next )
　　　　　{
　　　　　　　if( new->grade <= now->next->grade ) break;
　　　　　}

-----------------------------------------------------
/* できあがったリストの内容を先頭から順に表示する
ただし，番兵ノードは表示しない */
now = top;

　　→番兵ノードも表示されてしまうので、
　　　now = top->next;
　　　にします。

-----------------------------------------------------
全体的にポインタのチェックがありません。
きちんとヌルチェックをしたほうがいいです。

回答No.1

επιστημη（@episteme）

> while(new->grade > now->grade){
> 　now = now->next;
> }
> insertNext(now, new);

ここでおかしくなりそう。
このループが必ず停止するために番兵を置いたはず。
ならば番兵はどんな要素より大きくなくてはならない。
にもかかわらず、番兵は最も小さい。

さらに、番兵が最も大きいなら番兵は常にリストの末尾にあるはず。
そうであるなら、> insertNext(now, new); はnewをnowの"手前"に挿入しなければならない。