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

Version imprimable

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:

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:

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:

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:

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?

21/04/2014, 23h16
Obsidian
Bonjour et bienvenue,

Citation:

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:

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:

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 ».

Citation:

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.
22/04/2014, 00h59
Kairos
Bonsoir et merci de ta réponse :D
Citation:
Envoyé par Obsidian

Code:

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().
Citation:
Envoyé par Obsidian

Code:

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 :roll:). 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:

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 :P), cela reste ~~parfaitement~~ assez compréhensible à mon avis, et j'aime aussi profiter de la flexibilité syntaxique d'un langage :D. 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é.
22/04/2014, 08h52
pfeuh

Salut,

Citation:

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.

Citation:

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

Citation:
Envoyé par Kairos
main.c
Code:

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.

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 ^^'

Citation:

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 :mrgreen:
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:

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:

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;
}

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

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:

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.

22/04/2014, 20h18
Kairos

Bonjour,

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

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 :roll:. 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.
22/04/2014, 20h56
Médinoc

Oui, c'est ça.
22/04/2014, 22h15
Kairos

Très bien, merci beaucoup pour vos réponses. Ce fut assez instructif :roll: et je ne pense pas qu'il y ait grand chose à rajouter.
Je passe en résolu.

Merci encore et bonne soirée :D