Discussion :

[hashille]

Bonjour,
Comme indiqué dans l'intitulé, j'essaye de récupérer un texte provenant d'un fichier ligne par ligne et stocker les lignes dans un tableau de char*.
La compilation s'effectue sans problème, mais voici ce que j'obtiens quand j'essaye d'exécuter :

1;2 //
2;3 // cela marche bien pour les deux premières lignes mais après :

*** Error in `./a.out': double free or corruption (fasttop): 0x0000000001e682c0 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x7c8dc)[0x7fa1399ae8dc]
/lib64/libc.so.6(+0x87789)[0x7fa1399b9789]
/lib64/libc.so.6(cfree+0x16e)[0x7fa1399bf0ee]
./a.out[0x40081d]
./a.out[0x40088e]
/lib64/libc.so.6(__libc_start_main+0xea)[0x7fa13995250a]
./a.out[0x40067a] [...]


voici mon code si vous voulez bien jeter un coup d'oeil...
d'avance merci !

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
 
char **create_tmp(char *str, int line)
{
        FILE *file;
        char **data;
        char *buf;
        size_t size;
        int i = 0;
        int j = 0;
 
//      is_empty(str);                                                                                 
        file = fopen(str, "r");
        data = malloc(sizeof(char *) * (line + 1));
        if (data = NULL)
                return(NULL);
	while (i < line) {
     		buf = NULL;
                j = getline(&buf, &size, file);
	        if ( j == -1)
		        return(NULL);
                data[i] = malloc(sizeof(char) * (strlen(buf) + 1));
                if (data[i] == NULL)
                      return(NULL);
		strcpy(data[i], buf);
                data[i][strlen(buf) - 1] = '\0';
	        i++;
        }
        data[i] = NULL;
        fclose(file);
        return(data);
}
 
int main(int ac, char **av)
{
        char **tmp = NULL;
 
        tmp = create_tmp(av[1], 13);
}

Valgrind me dit que le problème vient du malloc ligne 19
PS : désolé je suis nouveau sur le forum ^^

[dalfab]

Bonjour,

Il faut utiliser les balises pour insérer du code (le bouton [#]) sinon c'est illisible.
Il y a des erreurs d'allocations/libérations de mémoire, des fonctions manquent il y en a certainement dedans.
Il y a utilisation d'un buffer libéré ligne ???? et utilisé immédiatement après en faisant my_putstr(buffer); ligne ????.

[hashille]

Bonsoir,
tout d'abord merci de m'avoir répondu, j'ai essayé de baliser le texte mais je ne trouve pas le résultat plus lisible ^^
Sinon toutes les fonctions sont là, j'ai enlevé le putstr que j'avais placé pour débug.

[dalfab]

Parmi les boutons au dessus de la fenêtre de saisie, c'est celui avec un # dessiné.

En effet, le nouveau code n'utilise plus la fonction my_strdup().
Ligne 15#, si buf est NULL, il ne faut normalement pas appeler free()
Ligne 16#, si on veut que getline() alloue la mémoire, il faut avoir mis NULL dans buf juste avant.
Ligne 18# et #21, attention aux return, le fclose() du fichier est perdu dans ces cas.
Le dernier buffer alloué par getline() n'est jamais libéré.

Je ne vois pas de problème ligne 19#, mais Valgrind est certainement plus fiable que moi.

[hashille]

Fait ! effectivement c'est plus lisible maintenant
Je prends note de tes commentaires mais ceux-ci ne semblent pas résoudre le problème, valgrind pointe vraiment du doigt l'allocation data[i]

[jlliagre]

if (data = NULL)

[hashille]

if (data = NULL)

Moyen d'être plus explicite ?
Si j'envoie un fichier vide, il n'y'aura pas de lignes et le malloc renverra donc nul, non ?

[Bousk]

= est l'assignation, == est le test.

[hashille]

Oh putain la boulette.
J'suis vraiment tard des fois ^^
Merci bien

[Sve@r]

Bonjour

Si j'envoie un fichier vide, il n'y aura pas de lignes et le malloc renverra donc nul, non ?

Non, il n'y a aucun lien entre "lire un fichier vide" et "erreur d'allocation". A la ligne 12 tu ouvres le fichier, à la ligne 13 tu demandes de la mémoire, ben que le fichier soit vide ou pas n'influera pas sur la demande.
Accessoirement, c'est dommage d'avoir ouvert le fichier avant la demande d'allocation (qui peut échouer et tu quittes avec un fichier ouvert)...

Maintenant question code, c'est assez moyen (surtout la gestion des erreurs d'allocation). Si un malloc échoue, tu quittes sans avoir libéré ce qui avait été alloué. Pareil pour toutes tes allocation avec getline et jamais libérées.
Et inutile de mettre le '\0' car strcpy() le fait pour toi.

Accessoirement, est-ce que tu es obligé de passer le nombre de lignes, Tu ne peux pas le trouver en les comptant dans le fichier ?
Suffit que tu rajoutes ceci en début de fonction

Code c :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
int c;
int line=0;
while((c=fgetc(file)) != EOF) {
    if (c == '\n') line++;
}
rewind(file);

(et donc là évidemment il faut ouvrir le fichier avant de faire la demande d'allocation !!!)

[hashille]

Ok je vais essayer de revoir les problèmes d'allocations mémoires, merci pour ton commentaire !
En effet j'avais déjà créé une fonction pour compter le nombres de lignes, j'ai isolé cette fonction car elle me posait problème et lui ai envoyé un nombre de lignes prédéterminé.

[jlliagre]

Il faudrait aussi que tu clarifies ce que tu souhaites faire avec la fonction create_tmp (qui mériterait sûrement un meilleur nom).

Puisque deuxième paramètre est un nombre de lignes, l'allocation d'un nombre de caractères qui est effectuée n'a pas de sens.

[Sve@r]

Puisque deuxième paramètre est un nombre de lignes, l'allocation d'un nombre de caractères qui est effectuée n'a pas de sens.

Ben pour moi si !!!
Il veut stocker "line" lignes, il alloue donc "line" pointeurs (en fait il alloue "line+1" parce qu'il veut stocker un NULL final mais le sens reste le même).
Ensuite chaque pointeur sera alloué pour stocker la ligne lue.
Non ???

En revanche, (et je viens d'y penser à l'instant), puisque getline() alloue lui-même chaque zone mémoire qu'elle remplit ensuite, pas besoin alors de réallouer une seconde zone pour y recopier la première. Autant récupérer directement le pointeur alloué par getline() !!!

Ca donnerait alors un truc du genre

Code c :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
char **create_tmp(char *str)
{
	FILE *file;
	char **data;
	int i;
	int line;
	size_t size=0;
 
	file = fopen(str, "r");
	if (file == NULL) return NULL;
 
	line=0;
	while ((i=fgetc(file)) != EOF) {
		if (i == '\n') line++;
	}
 
	data = calloc(line + 1, sizeof(char*));			// Le calloc met tout à 0 - Pas besoin de NULL final et chaque data[i] étant à 0 sera alors alloué par getline(). Et si on doit tout libérere, alors free(NULL) est autorisé
	if (data == NULL) {
		fclose(file);
		return NULL;
	}
 
	rewind(file); 
	for (i=0; i < line; i++) {
		if (getline(&data[i], &size, file) == -1) {
			fclose(file);
			for (i=0; i < line; i++) {
				free(data[i]);			// Si la zone est allouée elle est libérée, sinon elle est à NULL par le calloc initial et donc free(NULL) ne fait rien
			}
			free(data);
			return NULL;
		}
        }
        fclose(file);
        return data;
}

[jlliagre]

Ah oui, je n'avais pas regardé d'assez près le code.

Tout ça me parait un peu laborieux...

On pourrait lire le fichier d'un coup, ou même juste faire un mmap privé puis mettre des \0 à la place des newlines et affecter les pointeurs au fur et à mesure...

[hashille]

Merci pour les commentaires, très intéressant !
Je connaissais pas mmap private ça à l'air sympa j'y jetterai un oeil

[Sve@r]

On pourrait lire le fichier d'un coup, ou même juste faire un mmap privé puis mettre des \0 à la place des newlines et affecter les pointeurs au fur et à mesure...

Testons ça pour voir...

Code c :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
char **create_tmp(char *str)
{
	int file;
	char **data;
	char *map;
	int line;
	char c;
	size_t size;
	size_t i;
 
	file = open(str, O_RDONLY);
	if (file == -1) return NULL;
 
	// Compter les lignes et la taille (ici optimisation possible en lisant des blocs de "n" octets plutôt que 1 octet)
	for (size=0, line=0; read(file, &c, 1) > 0; size++) {
		if (c == '\n') line++;
	}
 
	// Allocation "line" pointeurs (+1 pour le NULL)
	data=malloc((line+1) * sizeof(char*));
	if (data == NULL) {
		close(file);
		return NULL;
	}
	data[line]=NULL;
 
	// Mapping fichier
	map=mmap(NULL, size, PROT_WRITE, MAP_PRIVATE, file, 0);
	close(file);
 
	// Transformer chaque newline en '\0' et récupérer le pointeur de la ligne
	line=0;
	for (i=0; i < size; i++) {
		if (map[i] == '\n') map[i]='\0';
		if (i == 0 || map[i-1] == '\0') {
			data[line]=&map[i];
			line++;
		}
	}
 
	return data;
}

Laborieux aussi non ??? Pourtant je ne pense pas qu'on puisse écrire cette solution de façon plus simple...
Petit avantage: on évite les allocations de chaque ligne...

[jlliagre]

Je pensais plutôt à ça :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
char **create_tmp1(char *filename)
{
  char **lines=NULL;
  char *p,*eol;
  int line=0;
  size_t size;
  struct stat stat;
  int fd=open(filename,O_RDONLY);
  if(fd!=-1) {
    if(fstat(fd, &stat)!=-1) {
      size = stat.st_size;
      p=mmap(0, size, PROT_WRITE, MAP_PRIVATE, fd, 0);
      close(fd);
      lines=malloc(sizeof(char*)*(size+1));     // il ne pourra pas y avoir plus de lignes que d'octets...
      p[size-1]='\0';                           // on postule que le dernier caractère du fichier est une fin de ligne
      while((eol=strchr(p,'\n'))) {             // tant qu'il reste des lignes
          lines[line++]=p;                      // on mémorise la ligne courante
          *eol='\0';                            // on la termine
          p=eol+1;                              // et on passe à la ligne suivante
      }
      lines[line++]=p;                          // on mémorise la dernière ligne
      lines[line++]=NULL;                       // et place le marqueur de fin
      lines=realloc(lines, sizeof(char*)*line); // puis ajustement du tableau à la bonne taille
      return(lines);
    }
  }
  return(NULL);
}

[Sve@r]

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
 
      lines=malloc(sizeof(char*)*(size+1));     // il ne pourra pas y avoir plus de lignes que d'octets...

Intelligent

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

      while((eol=strchr(p,'\n'))) {             // tant qu'il reste des lignes

Hum... là je reste dubitatif. A chaque ligne trouvée, strchr() relit tout depuis le début. Imaginons un fichier contenant 1000c où chaque caractère serait un '\n', il y aura 1000 * 1001 / 2 itérations...

[jlliagre]

Hum... là je reste dubitatif. A chaque ligne trouvée, strchr() relit tout depuis le début. Imaginons un fichier contenant 1000c où chaque caractère serait un '\n', il y aura 1000 * 1001 / 2 itérations...

Non, cette méthode ne relit jamais deux fois la même chose. C'était l'objectif du traitement en une passe. Tu as loupé le p=eol+1.

[Matt_Houston]

Une petite variation incrémentale (et portable) :

Code numl.c :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
 
 
#define ERROR_INVOKE 1
#define ERROR_INPUT  2
#define ERROR_IO     3
#define ERROR_MEMORY 4
 
#define CHUNK_SIZE (size_t)65536
 
#define TOKEN_EOL '\n'
 
 
typedef unsigned char byte_t;
 
 
/* outputs line number then line contents */
void prefix_lines(FILE *output, const void *buf, size_t size) {
    const byte_t *const init = buf;
    const byte_t *start = init,
                 *eol;
 
    size_t count = 1;
    do {
        const size_t rem = (size_t)(init + size - start);
        eol = memchr(start, TOKEN_EOL, rem);
        const size_t len = !eol ? rem : (size_t)(eol + 1 - start);
 
        fprintf(output, "%02zu: %.*s", count++, (int)len, (char *)start);
 
        start += len;
    } while (eol);
 
    fputs("\n", output); // last EOL
}
 
 
int main(int argc, char *argv[]) {
    FILE *input;
    int own = 0;
    switch (argc) {
    case 1:
        input = stdin;
        break;
 
    case 2:
        input = fopen(argv[1], "rb");
        if (!input) {
            perror("fopen");
            return ERROR_INPUT;
        }
        own = 1;
        break;
 
    default:
        puts("usage: numl [filename]");
        return ERROR_INVOKE;
    }
 
    int result = 0;
 
    byte_t *buf = NULL;
    size_t size = 0,
           chunk = CHUNK_SIZE;
    do {
        size += chunk;
        byte_t *const new = realloc(buf, size);
        if (!new) {
            perror("realloc");
            result = ERROR_MEMORY;
            goto end;
        }
        buf = new;
    } while ((chunk = fread(buf + size - CHUNK_SIZE, 1, CHUNK_SIZE, input)) == CHUNK_SIZE);
 
    if (!feof(input) || ferror(input)) {
        perror("fread");
        result = ERROR_IO;
        goto end;
    }
 
    size -= CHUNK_SIZE - chunk;
 
    prefix_lines(stdout, buf, size);
 
end:
    free(buf);
    if (own)
        fclose(input);
 
    return result;
}

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
$ tail -n +19 numl.c | head -n 19 | ./numl
01: /* outputs line number then line contents */
02: void prefix_lines(FILE *output, const void *buf, size_t size) {
03:     const byte_t *const init = buf;
04:     const byte_t *start = init,
05:                  *eol;
06: 
07:     size_t count = 1;
08:     do {
09:         const size_t rem = (size_t)(init + size - start);
10:         eol = memchr(start, TOKEN_EOL, rem);
11:         const size_t len = !eol ? rem : (size_t)(eol + 1 - start);
12: 
13:         fprintf(output, "%02zu: %.*s", count++, (int)len, (char *)start);
14: 
15:         start += len;
16:     } while (eol);
17: 
18:     fputs("\n", output); // last EOL
19: }
20: