Discussion :

[transgohan]

Bonjour,

je rencontre un problème lors de l'analyse d'un défaut de code.
Nous utilisons klocwork pour les analyses de code statique et il me remonte une erreur sur le code suivant :

Code c :

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

1
2
3
char chaine[5];
unsigned char i = 10;
sprintf(chaine, " %02d ", i);

Je tourne chèvre... Vous êtes bien d'accord qu'on ne peut pas dépasser la taille du buffer ? (unbound sprintf)
Ou bien j'ai loupé un truc dans le fonctionnement du sprintf ?

Car si c'est le cas je m'oriente vers un bug klocwork qui n'interprète pas correctement le %02d (ça va être sympa à justifier au client...).

Note : pas de snprintf dans ma bibliothèque.

[Sve@r]

Salut

Vous êtes bien d'accord qu'on ne peut pas dépasser la taille du buffer ? (unbound sprintf)

Hé non, je ne suis pas d'accord
Le "%02d" va demander un complément avec des "zéros" pour remplir 2 digits si le nombre n'en fait qu'un (ex 3 est affiché "03") mais ne tronque absolument pas un nombre qui en ferait plus. Et donc si "i" vaut 255 (unsigned char) il reste affiché "255". Plus l'espace avant plus l'espace après cela fait 5 caractères. Et si on rajoute le '\0', tout ça à stocker dans un char [5].
En plus (enfin là c'est juste une hypothèse), le "%d" demande à printf() de considérer cet argument comme un int donc entre (au mieux) -32768 et 32767, soit déjà 6 caractères avec le "-", plus un 7° avec le '\0' et (oserais-je te rappeler qu'il y a aussi des espaces ? ) tout ça dans un char [5] donc possible que klocwork arrive à une conclusion équivalente sans se préoccuper de la nature "char" de "i" =>

[transgohan]

Merci de cet éclairage bienvenu Sve@r !
Tellement peu l'habitude de travailler avec cette fichue fonction que j'ai mal interprété la spécification de format...

Du coup je me suis dit que j'allais tester ceci :

Code c :

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

1
2
3
char chaine[5];
unsigned char i = 10;
sprintf(chaine, " %02d ", i % 99);

2 (espaces) + 2 (taille i) + 1 (\0) = 5
C'est crade (perte potentielle d'information, même si dans mon cas on ne pourra effectivement pas avoir plus de 99) et en plus ça marche pas...

Je suis toujours mal éclairé ou tu es d'accord avec ma correction ?

Autre test pour tenter de comprendre même si cela n'apporte rien à mon code :

Code c :

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

1
2
3
4
5
6
char chaine[5];
unsigned char i = 10;
if( i < 100 )
{
   sprintf(chaine, " %02d ", i);
}

Défaut toujours présent...
Là j'ai vraiment l'impression qu'il s'en fiche et qu'il traite le défaut d'une manière générale...
Pourtant pour les défauts de dépassement mémoire sur les tableaux il sait très bien interpréter les branches de condition...
J'ai l'impression qu'il ne le fait pas pour les paramètres d'entrée du sprintf...

Je vais finir par lui affecter octet par octet si je trouve pas de solution lisible...

[Bktero]

Nous utilisons klocwork pour les analyses de code statique et il me remonte une erreur sur le code suivant

Il faut toujours rester prudent sur les analyseurs statiques. Ils sont écrits pas des humains et ont aussi leur limite. Ce que tu demandes n'est pas forcément trivial (la preuve, tu as demandé sur un forum et on t'a donné une explication bien plus longue que ce que tu pensais obtenir).

Code :

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

1
2
3
4
5
6
char chaine[5];
unsigned char i = 10;
if( i < 100 )
{
   sprintf(chaine, " %02d ", i);
}

Ici, l'analyse est relativement simple pour un humain, mais pas forcément pour un algorithme. Souvent, les analyseurs statiques ont du mal à avoir autant de contexte sur une ligne.

C'est aussi pour ça que tous (?) les analyseurs ont des possibilités pour ignorer certains warnings (soit avec une option pour supprimer toutes les vérifications d'une règle, soit pour marquer une ligne particulière de ton code comme étant un "faux-positif").

[Sve@r]

C'est crade (perte potentielle d'information, même si dans mon cas on ne pourra effectivement pas avoir plus de 99) et en plus ça marche pas...
Je suis toujours mal éclairé ou tu es d'accord avec ma correction ?

Je suis d'autant plus d'accord que j'étais arrivé au même résultat. Je pensais en effet qu'un modulo (i%100 en réalité si tu veux conserver les valeurs entre 0 et 99 inclus) serait la bonne idée si chaine ne peut pas grandir.

Alors j'ai une autre hypothèse: le "%d" demande un nombre signé, c'est à dire avec le signe "-" s'il est nécessaire. De là, un nombre négatif même à 2 chiffres occupera 3 espaces, plus les deux espaces et le '\0' et on dépasse 5. Enfin je reproduis là ce que pourrait penser klocwork. Donc déjà moi je mettrais "%02hu" (le "h" c'est une habitude chez-moi de toujours affiner le format mais "%02u" devrait pouvoir faire l'affaire). Et si vraiment ce con ne pige toujours pas, alors rajouter un cast ce qui donnerait sprintf(chaine, " %02hu ", (unsigned char)(i%100)). Tu peux aussi tenter la même chose avec ton second code en remplaçant if (i < 100) par if (i >= 0 && i < 100).

Je vais finir par lui affecter octet par octet si je trouve pas de solution lisible...

Alors dans ce cas essayer de travailler à l'envers et trouver à partir de combien klocwork est content. Donc essayer char chaine[10], puis char chaine[20] etc. Une fois trouvé, un sprintf() dans une chaine temporaire calée à la taille qui lui convient complété au final d'un strncpy() dans la vraie char chaine[5]. Sans oublier le commentaire adéquat pour que les autres comprennent le but de la manoeuvre.

[transgohan]

Bon beh tout remplacement effectué donne le même résultat...
Si quelqu'un pense à autre chose je prends !

C'est aussi pour ça que tous (?) les analyseurs ont des possibilités pour ignorer certains warnings (soit avec une option pour supprimer toutes les vérifications d'une règle, soit pour marquer une ligne particulière de ton code comme étant un "faux-positif").

C'est pas le plus compliqué en effet de tagguer comme faux-positif les 52 défauts.
Le plus compliqué c'est l'exigence cliente qui fait que pour chaque faux-positif je dois rédiger un rapport avec test exécutable prouvant qu'il n'y a pas de défaut...
Bref du temps bien employé... Et bien sûr un rapport+test par défaut, même si ce sont les mêmes...

C'est pour cela que je cherche par tout moyen à virer un maximum de défaut quitte à provoquer une mauvaise analyse dans certains cas pour que le défaut ne soit pas visible.

Alors dans ce cas essayer de travailler à l'envers et trouver à partir de combien klocwork est content.

Je n'ai pas ce luxe de mémoire RAM.
Il me reste moins de 1ko de mémoire disponible et j'ai encore des évolutions à venir.
Et je n'ai pas non plus accès à strncpy().

La joie des très vieux projets embarqués.

[dalfab]

Bonjour,

un tableau de 5 caractères prend exactement la même place qu'un tableau de 6 caractères car ici il doit être aligné avec la pile.
Ne peux-tu pas essayer d'utiliser la valeur char chaine[5+1];, avec le sprintf( chaine, " %02hu ", i ); ça pourrait marcher.

[transgohan]

Ce n'est pas un problème de 1 octet manquant mais comme l'indique Sve@r dans le second message du sujet il s'agit de 3-4 octets.
Sauf que je n'ai pas le luxe d'agrandir pour rien mes variables juste pour faire plaisir à un analyseur statique de code qui donne un faux positif...
J'ai déjà des algorithmes qui éclataient la pile et dont j'ai du migrer des variables en statique... Et ça c'est de la perte sèche de mémoire !

[sambia39]

Bonjour,

Je pense le contraire. Klocwork considère que sprintf n'est pas sûr et pour cause. Il n'y a aucune garantie sur la taille des données pour éviter par exemple des débordements. Il va donc tout simplement (quitte à ne pas dire systématiquement) émettre un warning sur l'utilisation de la fonction sprintf, car cette dernière figure sur sa liste des fonctions considérées comme non-sûres.

Ainsi, il ne faudrait pas interpréter ce warning comme étant un faux positif, mais plus comme étant un choix volontaire de signalement par Klocwork de toutes fonctions qu'il ne considère pas sûres et qu'il faut proscrire, car dans une mauvaise utilisation de la fonction, elle peut occasionner ou créer des vulnérabilités.

Au final, Klocwork vous force tout simplement à récrire vos instructions autrement ou à utiliser des fonctions alternatives sûres.

à bientôt.

[transgohan]

Il va donc tout simplement (quitte à ne pas dire systématiquement) émettre un warning sur l'utilisation de la fonction sprintf

En augmentant la taille du buffer klocwork ne remonte plus d'erreur critique (et non warning).
Mais je ne peux me le permettre dans mon projet qui manque cruellement de mémoire.

[Sve@r]

Il n'y a aucune garantie sur la taille des données pour éviter par exemple des débordements.

Si vraiment tu veux, tu peux. Tu mets un char chaine[100] et je te défie de la faire déborder à partir de sprintf(chaine, " %02d ", i)...

En augmentant la taille du buffer klocwork ne remonte plus d'erreur critique (et non warning).

Intéressant. A partir de quelle valeur minimale de "chaine[x]" klocwork est-il content ? Ca permettrait peut-être de découvrir comment il voit la chose...

Mais je ne peux me le permettre dans mon projet qui manque cruellement de mémoire.

C'est à ce point ? Non parce qu'on parle là juste de simples octets. A notre époque où les valeurs moyennes s'expriment en Go ça laisse réveur

[foetus]

Après tu peux faire 1 malloc ou passer 1 tableau/ zone statique (en embarqué, il me semble que cela se fait) pour essayer de perdre l'analyseur.

Mais avec 1 malloc, tu alloues dans le tas et non la pile.

[Sve@r]

Après tu peux faire 1 malloc pour essayer de perdre l'analyseur.

Ca risque de tellement le perdre qu'en voyant qu'on écrit cette fois dans un char étoile et non plus dans un tableau il va au contraire pêter une durite

[transgohan]

Après tu peux faire 1 malloc ou passer 1 tableau/ zone statique (en embarqué, il me semble que cela se fait) pour essayer de perdre l'analyseur.

Un malloc ? ça va pas la tête ?
Avec des problèmes de mémoire il est justement impératif de la gérer finement et d'assurer que jamais tu ne te retrouveras à court.
On implémente donc des wrappers de style malloc mais qui pointent vers des tableaux statiques comme zone mémoire dont la taille est fixe.
Cela nous permet d'évaluer l'empreinte mémoire à tout moment et de voir s'approcher le moment fatidique du plus de mémoire et appliquer des contournements si nécessaires.
Dans mon domaine, plus de mémoire implique plus d'équipement opérationnel, ce qui peut occasionner des morts... Donc on va éviter autant que possible.

Intéressant. A partir de quelle valeur minimale de "chaine[x]" klocwork est-il content ? Ca permettrait peut-être de découvrir comment il voit la chose...

Il prend le %d comme étant de taille 6 octets d'après mes tests. Donc, à priori, avec comme valeur max (en terme de caractères) : -32768

Et je confirme que lui passer un char* sans taille dans son scope de vérification il pète l'erreur systématiquement et ce même avec un tableau de 100...

C'est à ce point ? Non parce qu'on parle là juste de simples octets. A notre époque où les valeurs moyennes s'expriment en Go ça laisse réveur

Si je te dis que je travaille sur un processeur qui est plus vieux que moi (j'ai la trentaine) ça te laisse rêveur aussi ?
J'ai exactement 2Mo de RAM, sur un projet assez énorme... Toute optimisation est bonne à prendre quand on sait que le produit va perdurer pendant encore au moins 5-10ans.

[Sve@r]

Il prend le %d comme étant de taille 6 octets d'après mes tests. Donc, à priori, avec comme valeur max (en terme de caractères) : -32768

Héhé, ma première hypothèse. Il prend "%d" comme un int. Déçu toutefois que tu restes à "%d" (décimal) alors que "%u" donnerait au-moins l'indication "pas de signe -".
De là je pourrais te proposer sprintf(chaine, " %02hu ", i & 0x7f) pour voir s'il arrive à comprendre que "i" ne dépassera pas 64 (ok toi tu vas jusqu'à 99 mais déjà si on arrive à lui faire admettre cette étape on pourra continuer à partir de là).

Et je confirme que lui passer un char* sans taille dans son scope de vérification il pète l'erreur systématiquement et ce même avec un tableau de 100...

Dommage car ça t'aurait permis de tenter "&chaine[0]" (idée qui m'est venue suite au post de foetus). Tente le quand-même sait-on jamais...

[transgohan]

De là je pourrais te proposer sprintf(chaine, " %02hu ", i & 0x7f) pour voir s'il arrive à comprendre que "i" ne dépassera pas 64

Cela ne change rien, il ne doit pas considérer le format j'ai l'impression mais plutôt le type de la variable...

Sauf qu'en fait non... J'ai tenté ceci :

Code c :

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

1
2
3
unsigned char i = 10;
char chaine[4];
sprintf(chaine, "%hu", i);

Et il faut que je monte chaine à 7 pour que ça passe.
Du coup je ne vois même pas sur quoi il se base dans son analyse.

Dommage car ça t'aurait permis de tenter "&chaine[0]" (idée qui m'est venue suite au post de foetus). Tente le quand-même sait-on jamais...

Déjà tenté et cela ne change rien à l'analyse.

[foetus]

Et il faut que je monte chaine à 7 pour que ça passe.

Simple %hu c'est 1 type "unsigned short int" donc 1 valeur entre 0 et 65535.

65535, c'est 5 chiffres + 1 pour la sentinelle '\0'. Il reste 1 caractère pour le signe malgré 1 nombre positif ? pour la sécurité ? 1 peu des 2 ? autre ?

[transgohan]

Ah oui effectivement pas de format pour un unsigned char, c'est de l'int minimum...

Je pense que l'octet supplémentaire est pour le signe, qui peut le plus peut le moins doit être sa philosophie.

[Sve@r]

Ah oui effectivement pas de format pour un unsigned char, c'est de l'int minimum...

C'est pour ça que je tentais des trucs comme i%0x0f et que t'as tenté i%99. Mais ce con pige que dalle à ce qu'on veut lui faire comprendre.
Désolé perso je n'ai plus d'idée. Bktero a fait remarquer qu'un analyseur a ses limites et je sens que t'as trouvé les siennes. Ceci dit t'as pas choisi la facilité: un analyseur de m...imparfait sur un code placé sur un truc tellement vieux que t'en es à l'octet près. Ca me rappelle le film "apollo 13" où à un moment donné, le pilote qui était resté au sol suite à un rhume cherche à trouver une séquence d'allumage à partir d'un circuit ne pouvant pas dépasser 0.5A. Sauf que là ça se passait en 1970.

[Bktero]

Un malloc ? ça va pas la tête ?
Avec des problèmes de mémoire il est justement impératif de la gérer finement et d'assurer que jamais tu ne te retrouveras à court.
On implémente donc des wrappers de style malloc mais qui pointent vers des tableaux statiques comme zone mémoire dont la taille est fixe.
Cela nous permet d'évaluer l'empreinte mémoire à tout moment et de voir s'approcher le moment fatidique du plus de mémoire et appliquer des contournements si nécessaires.

T'as cru que malloc alloue magiquement de la mémoire sans savoir où il va ?

Un tas, c'est un gros tableau statique donc la taille est fixe. J'ai déjà vu des gens utilisés les points d'extension de leur toolchain pour vérifier les allocations dynamiques, en pimpant l'allocateur de base, et en continuant d'utiliser malloc() et free() dans leur code.

Pour ce qui est de la criticité d'un manque de mémoire, cela dépend forcément de l'application. Même dans une application critique, il peut y avoir des parties non critiques ; si dans ces parties tu manques de mémoire, tu peux certainement te tourner vers un contournement. Exmple : tu n'arrives pas à allouer assez de mémoire pour afficher un texte sur un écran ? Tu peux tronquer le message.

Enfin, et c'est le plus important, le vrai problème de l'allocation dynamique est la fragmentation mémoire. Exemple : tu veux allouer un bloc 100 bytes. Il y a au total 1'000 bytes libres dans le tas, mais il n'y a pas 100 bytes contiguës. Ton allocation échoue. Tu ne peux rien faire contre ça. C'est très compliqué à prédire car passer par tous les chemins possibles de l'application n'est pas suffisant. C'est compliqué de s'en protéger car augmenter la taille du tas ne règle pas le problème (cela repousse juste le moment où ça peut arriver) et aucun allocateur custom ne pourra t'aider. Ou alors il te faut un garbage collector à la Java, mais on est sur un forum C, on va s'éloigner des (relativement) possibles