Mes pitites lignes de code sont-elles sûres jusqu'à présent ?

**Kairos** · 21/04/2014, 20h34

Bonjour,

Etant débutant dans la programmation en C et apportant une importance toute particulière à l'efficacité et la sûreté de ce que j’entreprends, j'aimerais avoir votre avis sur ce petit programme, ne faisant rien de particulier à part récupérer une entrée utilisateur via une fonction que j'ai écrite, tout en utilisant des allocations dynamiques.

main.c

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
 
#include "std.h"
#include "util.h"
/* #include "calc.h" */
 
#define SIZE 1024
 
int main(int argc, char** argv)
{
        const size_t buffer_size = sizeof(char)*(SIZE+1);
 
        char* expr  = (char*)calloc(buffer_size, sizeof(char));
        char* input = (char*)calloc(buffer_size, sizeof(char));
 
        if (argc > 1 && strlen(argv[1]) > 0)
                strcpy(expr, argv[1]);
        else {
                printf("Enter an expression: ");
                if (GetUserInput(input, buffer_size, '\n', 1))
                        strcpy(expr, input);
                else
                        strcpy(expr, "0");
        }
 
        if (expr != "0") {
                printf("Attempting to calculate '%s'\n", expr);
                /* CalcExpr(expr, buffer_size); */
        }
 
        free(expr);
        free(input);
 
        return 0;
}

util.c

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
 
#include "std.h"
#include "util.h"
 
unsigned short GetUserInput(char* buffer, const size_t s_buffer, char c_stop, unsigned short is_dynamic)
{
        size_t i;
        char c;
 
        if (s_buffer > 0) {
                for (i = 0; i < s_buffer && (c = getchar()) != (c_stop ? c_stop : '\n'); ++i) {
                        buffer[i] = c;
                }
 
                buffer[i] = '\0';
 
                if (is_dynamic && i < s_buffer)
                        return realloc((char*)buffer, sizeof(char)*(i+1)) ? 1 : 0;
                else
                        return 1;
        } else
                return 0;
}

util.h

Code :

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

1
2
3
4
5
6
7
 
#ifndef UTIL_H_INCLUDED
#define UTIL_H_INCLUDED
 
unsigned short GetUserInput(char* buffer, const size_t s_buffer, char c_stop, unsigned short is_dynamic);
 
#endif

std.h

Code :

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

1
2
3
4
5
6
7
8
9
 
#ifndef STD_H_INCLUDED
#define STD_H_INCLUDED
 
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
 
#endif

Merci d'avance

PS: Est-il possible de mettre des champs "spoiler" dans les messages?

**Obsidian** · 21/04/2014, 23h16

Bonjour et bienvenue,

Envoyé par Kairos

Etant débutant dans la programmation en C et apportant une importance toute particulière à l'efficacité et la sûreté de ce que j’entreprends, j'aimerais avoir votre avis sur ce petit programme, ne faisant rien de particulier à part récupérer une entrée utilisateur via une fonction que j'ai écrite, tout en utilisant des allocations dynamiques.

Effectivement, tu as l'air d'apporter un soin particulier à la fois à ton code et à tes commentaires. C'est un plaisir de te lire.

Code :

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

1
2
3
4
        const size_t buffer_size = sizeof(char)*(SIZE+1);
 
        char* expr  = (char*)calloc(buffer_size, sizeof(char));
        char* input = (char*)calloc(buffer_size, sizeof(char));

Il y a quelque chose de redondant dans ces lignes. Tu alloues « sizeof (char) » fois plus de mémoire que nécessaire.

Code :

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

        if (expr != "0") {

Ça, ça ne fonctionnera pas. En faisant cela tu compares deux pointeurs, c'est-à-dire les adresses respectives en mémoire de ton buffer, d'une part, et de la chaine de référence « "0" » d'autre part, qui se trouve dans une zone de mémoire en lecture seule. Le résultat sera donc toujours faux. Utilise strcmp() et strncmp() à la place. S'il n'y a qu'un seul caractère à vérifier, tu peux également déréférencer directement les pointeurs avec « *expr ».

PS: Est-il possible de mettre des champs "spoiler" dans les messages?

Pas à ma connaissance, mais certaines personnes ici choisissent d'écrire avec la couleur blanche. Il suffit ensuite de faire une sélection à la souris pour révéler les mots en question.

**Kairos** · 22/04/2014, 00h59

Bonsoir et merci de ta réponse

Envoyé par Obsidian

Code :

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

1
2
3
4
5
 
        const size_t buffer_size = sizeof(char)*(SIZE+1);
 
        char* expr  = (char*)calloc(buffer_size, sizeof(char));
        char* input = (char*)calloc(buffer_size, sizeof(char));

Il y a quelque chose de redondant dans ces lignes. Tu alloues « sizeof (char) » fois plus de mémoire que nécessaire.

Petite faute de ma part, en effet. Je peux directement passer le buffer expr en argument à ma fonction GetUserInput().

Envoyé par Obsidian

Code :

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

1
2
 
        if (expr != "0") {

Ça, ça ne fonctionnera pas. En faisant cela tu compares deux pointeurs, c'est-à-dire les adresses respectives en mémoire de ton buffer, d'une part, et de la chaine de référence « "0" » d'autre part, qui se trouve dans une zone de mémoire en lecture seule. Le résultat sera donc toujours faux. Utilise strcmp() et strncmp() à la place. S'il n'y a qu'un seul caractère à vérifier, tu peux également déréférencer directement les pointeurs avec « *expr ».

Ah, grosse négligence de ma part cette fois-ci. D'ailleurs, je me suis fait un peu plus tôt la remarque sur ce que pourrait renvoyer une chaîne 'brute' comme "0", j'ai justement pensé à un pointeur ciblant le premier caractère de cette même chaîne. Content de voir que je ne me suis pas trompé dans mon raisonnement ^^' (tout ceci est logique de toute manière

). J'aurais dû y penser en comparant expr à 0... Ceci dit, merci pour ta remarque, cela ne me serait probablement pas venu à l’esprit de tout simplement déréférencer expr, puisqu'il s'agit bien de comparer un seul caractère.

Une dernière chose, concernant la ligne ci-dessous plus précisément :

Code :

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

for (i = 0; i < s_buffer && (c = getchar()) != (c_stop ? c_stop : '\n'); ++i)

J'ai cette (mauvaise?) tendance à compresser mon code de cette manière. Je trouve ça plus... joli (chacun ses goûts

), cela reste ~~parfaitement~~ assez compréhensible à mon avis, et j'aime aussi profiter de la flexibilité syntaxique d'un langage

. Ce dont j'ai peur est la perte de performance que cela peut engendrer. Je sais pertinemment que s'il en existe une, elle serait négligeable dans ce cas-ci, surtout en C ou cette perte se traduirait par l’exécution de quelques opérandes supplémentaires j'imagine. Cependant, je n'ai vraiment pas envie de prendre de mauvaises habitudes.
Donc je me posais cette question : dois-je privilégier un style de codage un peu plus 'décousu' ou puis-je pleinement profiter de la liberté de syntaxe que m'offre ce merveilleux langage

, tant que cela reste assez compréhensible bien entendu ? Après, j'entends bien qu'en groupe, la clarté du code est un point primordial. Dans ce cas, il serait sans doute plus sage d'opter pour un style plus détaillé.

**pfeuh** · 22/04/2014, 08h52

Salut,

Envoyé par Kairos

dois-je privilégier un style de codage un peu plus 'décousu' ou puis-je pleinement profiter de la liberté de syntaxe que m'offre ce merveilleux langage

Pour moi la lisibilité prime tout. S'il y a des problèmes de performances, c'est seulement une fois validé qu'on "optimise" un code... A condition que cela apporte réellement un plus... Et qu'on laisse une trace du code original... par exemple une formule ou un pseudo-code en commentaire.

Envoyé par Kairos

tant que cela reste assez compréhensible bien entendu ?

Et où est la limite entre le compréhensible et le non compréhensible?

A+

Pfeuh

**dahtah** · 22/04/2014, 12h29

Envoyé par Kairos

main.c

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
13
#define SIZE 1024

int main(int argc, char** argv)
{
        const size_t buffer_size = sizeof(char)*(SIZE+1);

        char* expr  = (char*)calloc(buffer_size, sizeof(char));
        char* input = (char*)calloc(buffer_size, sizeof(char));

        if (argc > 1 && strlen(argv[1]) > 0)
                strcpy(expr, argv[1]);
        else {
        }

Attention, tu as un buffer overflow sur la heap (je pense que c'est exploitable ici si input est place après expr sur la heap, car on pourra ré-écrire input->next qui sera appelé par le free(input) final). Tu ne vérifies pas la longueur de argv[1], puis tu copies dans un tableau de taille fixe. argv[1] est par définition contrôlable par l'utilisateur, donc de longueur arbitraire.

En règle générale, n'utilise jamais strcpy (plus généralement, toutes les vielles fonctions unsafe gets, sprintf, ...). Préfère strlcpy (si dispo) ou strncpy.

**Kairos** · 22/04/2014, 14h04

Aïe... j'ai oublié de vérifier la longueur de argv[1]. Je vois le problème. Merci pour ta remarque, je ferai attention à ne pas call de "vieilles fonctions unsafe" dorénavant ^^'

Et où est la limite entre le compréhensible et le non compréhensible?

Écoute... tu soulèves là une bien bonne question... dont je ne saurai pas quoi répondre

J'essayerai de ne pas abusé de ces conditions ternaires (ou autres, si j'en trouve plus tard :p)

Sinon, vous pensez que tout va un peu mieux maintenant? J'ai debug mon programme avec un SIZE de 10, plus de buffer overflow à priori, mais je préfère être sûr et passer par vous si vous n'en voyez aucun inconvénient ^^':

main.c

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
#include "std.h"
#include "util.h"
/* #include "calc.h" */
 
#define SIZE 1024
 
int main(int argc, char** argv)
{
        const size_t buffer_size = sizeof(char)*(SIZE+1);
 
        char* expr = (char*)calloc(buffer_size, sizeof(char));
 
        if (argc > 1 && strlen(argv[1]) > 0)
                strncpy(expr, argv[1], buffer_size-sizeof(char));
        else {
                printf("Enter an expression: ");
                if (!GetUserInput(expr, buffer_size, '\n'))
                        expr = "0";
        }
 
        printf("Attempting to calculate '%s'\n", expr); /* J'ai abandonné la comparaison, je compte bien envoyer "0" à ma fonction CalcExpr() */
        /* CalcExpr(expr); */
 
        free(expr);
 
        return 0;
}

util.c

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
#include "std.h"
#include "util.h"
 
unsigned short GetUserInput(char* buffer, const size_t s_buffer, char c_stop)
{
        size_t i;
        char c;
 
        if (s_buffer > 0) {
                for (i = 0; i < s_buffer-sizeof(char) && (c = getchar()) != (c_stop ? c_stop : '\n'); ++i) {
                        buffer[i] = c;
                }
 
                buffer[i] = '\0';
 
                /* Plus de realloc() ici puisqu'il s'agit du buffer expr que je modifie, et non input. J'ai besoin de faire quelques transformations sur la chaîne contenue dans expr */
 
                return 1;
        } else
                return 0;
}

**Médinoc** · 22/04/2014, 17h29

Tu n'as toujours pas corrigé le début:

Code :

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

1
2
3
        const size_t buffer_size = sizeof(char)*(SIZE+1);
 
        char* expr = (char*)calloc(buffer_size, sizeof(char));

Correction suggérée:

Code :

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

1
2
3
        const size_t cch_buffer_size = SIZE+1; /*Taille de buffer en caractères*/
 
        char* expr = calloc(cch_buffer_size, sizeof(char)); /*calloc() s'occupe de faire la multiplication*/

Au passage, j'ai retiré le cast explicite en (char*), qui n'est pas nécessaire si tu compiles en C.
Et si tu compiles en C++, la correction c'est de compiler en C à la place, et non de changer ton code.

**Kairos** · 22/04/2014, 20h18

Bonjour,

Je compile bien en C, et non en C++

Il est vrai que j'explicite souvent certaines choses inutiles, par peur de faire une grosse bêtise comme je l'ai déjà fait auparavant en essayant de manipuler des pointeurs de fonction

. Enfin... passons ^.^

A propos des cast, lorsqu'une fonction attend par exemple un argument de type const char*, je n'ai pas besoin d’expliciter de cast à partir d'un char* normalement, n'est-ce pas? Pour la fonction appelée, l'argument sera en read-only uniquement, de ce fait elle ne sera modifiable qu'en dehors de cette fonction.