Besoin d'vais de programmeurs C (Je n'y connais rien en C)

"shrom" <shrom@admin.zzn.com> wrote in message
news:4249d581$0$26377$636a15ce@news.free.fr...

Bonjour,

Je suis novice sur le forum, et pour cause, je ne connais strictement
rien en C. J'ai besoin d'un petit script CGI qui concatène deux fichiers.

J'ai fait ça ce week end, ça compile, ça fonctionne mais je voudrais
l'avis de spécialistes sur le code.

Ton code est truffé de problèmes... Il serait préférable d'utiliser un autre
langage pour un but aussi simple.

__CODE__
#include<stdio.h>


main( int argc, char **argv, char **env ) {

mauvais prototype, utiliser int main(int argc, char **argv)

// Header HTTP obligatoire
printf("Content-type: text/htmlnn");

FILE *f1, *f2;
char path[255];
char ligne[255];

Les déclaration devraient être groupées en début de fonction, ou au moins de
bloc, pour une meilleure portabilité (C89) et une meilleure lisibilité.

f1 = fopen( "stats.txt" );

manque le deuxieme parametre : "r" ou "rb" si le fichier est binaire.

if ( f1 != NULL ) {

pas de diagnostique si le fichier n'existe pas ?

while( ! feof( f1 ) ) {

NON, cette boucle ne fonctionne pas, le test feof(f1) est inapproprié

fscanf( f1, "%sn", ligne);

catastrophe : buffer overflow, faille de sécurité...

printf("%sn", ligne);

d'ailleurs cela ne fonctionne pas sur la derniere ligne d'un fichier ne se
terminant pas par un n

}
fclose( f1 );
}

sprintf(path,"%s",getenv("PATH_TRANSLATED"));

encore un buffer overflow potentiel

f2 = fopen( path, "r" );
if ( f2 == NULL ) {
printf("Erreur d'ouverture du fichier.");

le diagnostique va dans la sortie standard, c'est brutal.

return 1;
}

while( ! feof( f2 ) ) {
fscanf( f2, "%sn", ligne);
printf("%sn", ligne);
}
fclose( f2 );
}

memes remarques que pour f1

__CODE__

Je crains le buffer overflow avec la variable buffer. Je pense qu'il y a
aussi un problème lorsqu'une ligne contient plus de 255 caratères.

Effectivement !

voici une alternative simple :

#include <stdio.h>

int catfile(const char *filename, const char *err1, const char *err2) {
FILE *fp;
int c;

if (filename == NULL) {
if (err1)
printf("%s", err1);
return 1;
}
if ((fp = fopen(filename, "rb")) == NULL) {
if (err2)
printf(err2, filename);
return 2;
}
while ((c = getc(fp)) != EOF)
putchar(c);
fclose(fp);
return 0;
}

int main(int argc, char **argv) {
/* Header HTTP obligatoire */
printf("Content-type: text/htmlnn");
catfile("stats.txt", NULL, NULL);
catfile(getenv("PATH_TRANSLATED",
"variable PATH_TRANSLATED non définie.n",
"Erreur d'ouverture du fichier %s.n"));
return 0;
}

Comme tu le constates, il est préférable de définir des fonctions pour des
tâches de base, plutôt que de dupliquer le code.
Contrairement aux idées reçues, la boucle de lecture getc()/putc() n'est pas
pénalisante, en particulier si ces fonctions sont implementées par des macros ou
inline comme il se doit.

Chqrlie.

PS: je n'ai pas compilé ce code.

Ton code est truffé de problèmes... Il serait préférable d'utiliser un autre
langage pour un but aussi simple.

En fait, j'utilise tout un tas de langages, mais je n'ai jamais réussi à
m'intéresser au C. Si j'ai choisi le C, c'est parce qu'il doit y avoir
une utilisation assez intensive du prog etje pensais que niveau perf, le
C était le plus approprié car le traitement est simple.

[SNIP]

f1 = fopen( "stats.txt" );

manque le deuxieme parametre : "r" ou "rb" si le fichier est binaire.

C'est une erreur lors de la recopie.

if ( f1 != NULL ) {

pas de diagnostique si le fichier n'existe pas ?

C'était volontaire pour ne pas poluer la sortie en cas de problème

while( ! feof( f1 ) ) {

NON, cette boucle ne fonctionne pas, le test feof(f1) est inapproprié

Chez moi ça fonctionne, mais je veuxbien te croire quand tu dis qu'elle
est inappropriée.

fscanf( f1, "%sn", ligne);

catastrophe : buffer overflow, faille de sécurité...

printf("%sn", ligne);

d'ailleurs cela ne fonctionne pas sur la derniere ligne d'un fichier ne se
terminant pas par un n

Je m'en doutais


[SNIP]

voici une alternative simple :

#include <stdio.h>

int catfile(const char *filename, const char *err1, const char *err2) {
FILE *fp;
int c;

if (filename == NULL) {
if (err1)
printf("%s", err1);
return 1;
}
if ((fp = fopen(filename, "rb")) == NULL) {
if (err2)
printf(err2, filename);
return 2;
}
while ((c = getc(fp)) != EOF)
putchar(c);
fclose(fp);
return 0;
}

int main(int argc, char **argv) {
/* Header HTTP obligatoire */
printf("Content-type: text/htmlnn");
catfile("stats.txt", NULL, NULL);
catfile(getenv("PATH_TRANSLATED",
"variable PATH_TRANSLATED non définie.n",
"Erreur d'ouverture du fichier %s.n"));
return 0;
}

Comme tu le constates, il est préférable de définir des fonctions pour des
tâches de base, plutôt que de dupliquer le code.
Contrairement aux idées reçues, la boucle de lecture getc()/putc() n'est pas
pénalisante, en particulier si ces fonctions sont implementées par des macros ou
inline comme il se doit.

Chqrlie.

PS: je n'ai pas compilé ce code.

Merci beaucoup pour cette réponse rapide et détaillée, je vais de ce pas
essayé ce code.

shrom wrote on 30/03/05 :

Bel effort, mais il y a quelques erreurs. Pour approfondir:

http://mapage.noos.fr/emdel/notes.htm


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

int main (void)
{
int ret = EXIT_SUCCESS;
static char const F_STAT[] = "stats.txt";
static char const VAR[] = "PATH_TRANSLATED";

/* Header HTTP obligatoire */
printf ("Content-type: text/htmlnn");

{
FILE *f1 = fopen (F_STAT, "r");

if (f1 != NULL)
{
char ligne[255];

while (fgets (ligne, sizeof ligne, f1))
{
printf ("%s", ligne);
}
fflush (stdout);
fclose (f1), f1 = NULL;

{
char *path = getenv (VAR);

if (path != NULL)
{
FILE *f2 = fopen (path, "r");

if (f2 != NULL)
{
while (fgets (ligne, sizeof ligne, f2))
{
printf ("%s", ligne);
}
fflush (stdout);
fclose (f2), f2 = NULL;
}
else
{
printf ("Fichier %s inconnun", path);
ret = EXIT_FAILURE;
}
}
else
{
printf ("Variable %s inconnuen", VAR);
}
}
}
else
{
printf ("Fichier %s inconnun", F_STAT);
ret = EXIT_FAILURE;
}
}
return ret;
}

--
Emmanuel
The C-FAQ: http://www.eskimo.com/~scs/C-faq/faq.html
The C-library: http://www.dinkumware.com/refxc.html

"There are 10 types of people in the world today;
those that understand binary, and those that dont."

shrom wrote:

Bonjour,

Je suis novice sur le forum, et pour cause, je ne connais strictement
rien en C. J'ai besoin d'un petit script CGI qui concatène deux
fichiers.

J'ai fait ça ce week end, ça compile, ça fonctionne mais je voudrais
l'avis de spécialistes sur le code.

Quelques remarques en plus de celles qui ont déjà été données :

Pour ne pas chercher un programme déjà existant (comme 'cat' sur un
Unix) ?

Il faut toujours se méfier des programmes qui ont l'air de fonctionner.

(...)

char path[255];
C'est un peu petit pour un path, il est de toute manière préférable de

se passer de cette variable.
(et pourquoi 255 et pas 256 ?)

(...)

fscanf( f1, "%sn", ligne);
printf("%sn", ligne);

Outre le buffer-overflow qui pend au nez de fscanf() Il est complètement
inutile de faire des entrées-sorties formatées avec fscanf() et
fprintf() qui dépensent du CPU sans raison, utiliser fgets ou mieux
getc et putchar ce qui décharge des problèmes d'overflow.

(...)

sprintf(path,"%s",getenv("PATH_TRANSLATED"));
f2 = fopen( path, "r" );

Il n'est pas utile de passer par la variable path, cela fait une
occasion supplémentaire d'overflow en dépensant inutilement du CPU :

f2 = fopen( getenv( "PATH_TRANSLATED" ), "r" ) ) ;
est suffisant.

Ou alors, pour détailler, si le path doit être affiché en cas d'erreur à
l'ouverture par exemple :
char * path ;
...
path = getenv( "PATH_TRANSLATED" ) ; // path pointe sur le path
f2 = fopen( path, "r" ) ;
if ( f2 == NULL )
{
fprintf( stderr, "Erreur à l'ouverture de %sn", path ) ;
return( 1 ) ; // ou quelqu'autre valeur indiquant une erreur
}

Je crains le buffer overflow

Il faut toujours craindre le buffer overflow, et dans ce cas il y avait
de bonnes raisons.

Bref, la solution donné par Charlie G. est sans doute ce qu'il y a de
mieux à faire.

--
Patrick http://patrick.davalan.free.fr/

Harpo wrote on 30/03/05 :

f2 = fopen( getenv( "PATH_TRANSLATED" ), "r" ) ) ;
est suffisant.

si getenv() renvoi NULL, t'es mal...

--
Emmanuel
The C-FAQ: http://www.eskimo.com/~scs/C-faq/faq.html
The C-library: http://www.dinkumware.com/refxc.html

"C is a sharp tool"

Emmanuel Delahaye wrote:

shrom wrote on 30/03/05 :

Bel effort, mais il y a quelques erreurs. Pour approfondir:

http://mapage.noos.fr/emdel/notes.htm

Interessant, avec ta permission, je mettrai un lien sur mes pages.

Je reproche à ta solution d'utiliser fgets() alors qu'il n'y a aucun
interêt à utiliser une ligne, celle-ci ne faisant l'objet d'aucun
traitement particulier en tant que ligne.
De plus, pour bien faire, il faudrait gérer les possibles troncatures,
et 255 (pourquoi 255 ?) n'est pas très grand (surtout dans ce cas où il
s'agit d'html (parfois généré par des programmes qui ne prennent pas le
soin de mettre des n et d'indenter correctement, ces sauvages!)).
getc()/putchar() permettent d'éviter ces problèmes (débordement et
troncature) sans apporter d'overhead significatif (voire pas d'overhead
du tout) et le code est plus simple.

Faire une fonction pour faire 2 choses similaires me semblait aussi être
une bonne chose. (bon je ne me suis pas amusé à compiler le programme
de Charlie G. et à le tester, mais a première vue il a l'air correct.)

Le fait d'imbriquer autant les blocks est peut-être élégant mais ne
simplifie pas la lecture quand un block ne tient pas sur une page
d'écran et ici il n'y a pas d'interêt particulier à le faire. Utiliser
une fonction permet de simplifier la lecture. C'est aussi très
important, ama.

Sinon, quelle est dans le cas présent l'utilité du fflush( stdout ) ?
faire patienter le client ?

Dois-je présenter une solution avec des goto ? ;-)

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

int main (void)
{
int ret = EXIT_SUCCESS;
static char const F_STAT[] = "stats.txt";
static char const VAR[] = "PATH_TRANSLATED";

/* Header HTTP obligatoire */
printf ("Content-type: text/htmlnn");

{
FILE *f1 = fopen (F_STAT, "r");

if (f1 != NULL)
{
char ligne[255];

while (fgets (ligne, sizeof ligne, f1))
{
printf ("%s", ligne);
}
fflush (stdout);
fclose (f1), f1 = NULL;

{
char *path = getenv (VAR);

if (path != NULL)
{
FILE *f2 = fopen (path, "r");

if (f2 != NULL)
{
while (fgets (ligne, sizeof ligne, f2))
{
printf ("%s", ligne);
}
fflush (stdout);
fclose (f2), f2 = NULL;
}
else
{
printf ("Fichier %s inconnun", path);
ret = EXIT_FAILURE;
}
}
else
{
printf ("Variable %s inconnuen", VAR);
}
}
}
else
{
printf ("Fichier %s inconnun", F_STAT);
ret = EXIT_FAILURE;
}
}
return ret;
}

--
Patrick http://patrick.davalan.free.fr/

Emmanuel Delahaye wrote:

Harpo wrote on 30/03/05 :

f2 = fopen( getenv( "PATH_TRANSLATED" ), "r" ) ) ;
est suffisant.

si getenv() renvoi NULL, t'es mal...

Heu ... oui ...

--
Patrick http://patrick.davalan.free.fr/

Je vous remercie tous pour vos réponses.

J'ai testé le code de Charlie Gordon, il fonctionne bien ( une petite
remarque à la compilation sur un cast non explicite ).

Si j'ai le temps un jour, je me mettrai peut être au C, d'autant que les
réponses sur ce newsgroup sont rapides et courtoises.

PS: désolé pour les deux pseudo différents ( shrom et ftc ) mais je
poste de deux machines différentes et je n'avais pas fait attention à la
configuration du compte.

"Charlie Gordon" <news@chqrlie.org> writes:

Ton code est truffé de problèmes... Il serait préférable d'utiliser un autre
langage pour un but aussi simple.

du genre

#!/bin/sh
echo "Content-type: text/html"
echo
cat stats.txt $PATH_TRANSLATED


MB
--
Michel BILLAUD billaud@labri.fr
LABRI-Université Bordeaux I tel 05 4000 6922 / 05 5684 5792
351, cours de la Libération http://www.labri.fr/~billaud
33405 Talence (FRANCE)

Harpo wrote on 30/03/05 :

Bel effort, mais il y a quelques erreurs. Pour approfondir:

http://mapage.noos.fr/emdel/notes.htm

Interessant, avec ta permission, je mettrai un lien sur mes pages.

Aucun problème.

Je reproche à ta solution d'utiliser fgets() alors qu'il n'y a aucun
interêt à utiliser une ligne, celle-ci ne faisant l'objet d'aucun
traitement particulier en tant que ligne.

Je n'ai fait que mettre en forme le code du PO. On m'a souvent reproché
d'apporter des solutions trop radicales... Dur de satisfaire tout le
monde...

De plus, pour bien faire, il faudrait gérer les possibles troncatures,
et 255 (pourquoi 255 ?) n'est pas très grand (surtout dans ce cas où il
s'agit d'html (parfois généré par des programmes qui ne prennent pas le
soin de mettre des n et d'indenter correctement, ces sauvages!)).
getc()/putchar() permettent d'éviter ces problèmes (débordement et
troncature) sans apporter d'overhead significatif (voire pas d'overhead
du tout) et le code est plus simple.

C'est automatique. Lire la doc de fgets().

Faire une fonction pour faire 2 choses similaires me semblait aussi être
une bonne chose. (bon je ne me suis pas amusé à compiler le programme
de Charlie G. et à le tester, mais a première vue il a l'air correct.)

Si j'avais voulu refaire le truc, j'aurais fait probablement comme
Charlie. Mais encore une fois, je me suis contenté de faire tourner le
code du P.O. sans trop le hambugeriser...

Le fait d'imbriquer autant les blocks est peut-être élégant mais ne
simplifie pas la lecture quand un block ne tient pas sur une page
d'écran et ici il n'y a pas d'interêt particulier à le faire. Utiliser
une fonction permet de simplifier la lecture. C'est aussi très
important, ama.

C'est une préparation à la modularisation. Les 'fonctions' apparaissent
en filigrane. C'est une étape intermédiaire... Encore une fois, je me
suis retenu d'aller plus loin...

Sinon, quelle est dans le cas présent l'utilité du fflush( stdout ) ?
faire patienter le client ?

Je ne vois pas le rapport. Il n'est pas dit que la dernière ligne soit
terminée par un 'n', donc je force l'émission. Ca te choque ? Ca
mérite des sarcasmes aussi plats qu'inutiles ?

Dois-je présenter une solution avec des goto ? ;-)

Tu fais ce que tu veux...

--
Emmanuel
The C-FAQ: http://www.eskimo.com/~scs/C-faq/faq.html
The C-library: http://www.dinkumware.com/refxc.html

.sig under repair