Analyse statique : problème de unbound sprintf

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:

---

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. ;)

Salut

Citation:

---

Envoyé par transgohan

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 :mrgreen:
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]. :aie:
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 ? :D) tout ça dans un char [5] donc possible que klocwork arrive à une conclusion équivalente sans se préoccuper de la nature "char" de "i" => :aie::aie::aie:
:chin:

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:

---

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:

---

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... :calim2:

Citation:

---

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:

---

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").

Citation:

---

Envoyé par transgohan

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

Citation:

---

Envoyé par transgohan

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

---

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.

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

Citation:

---

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.

Citation:

---

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. :(

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.

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 !

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.

Citation:

---

Envoyé par sambia39

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.

Citation:

---

Envoyé par sambia39

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

Citation:

---

Envoyé par transgohan

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

Citation:

---

Envoyé par transgohan

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

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.

Citation:

---

Envoyé par foetus

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

Citation:

---

Envoyé par 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.

---

:alerte::alerte::alerte::pan::scarymov:
Un malloc ? ça va pas la tête ? :D
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. :aie:

Citation:

---

Envoyé par Sve@r

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

Citation:

---

Envoyé par Sve@r

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 ? :D
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.

Citation:

---

Envoyé par transgohan

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à).

Citation:

---

Envoyé par transgohan

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

Citation:

---

Envoyé par Sve@r

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:

---

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. :calim2:
Du coup je ne vois même pas sur quoi il se base dans son analyse. :marteau:

Citation:

---

Envoyé par Sve@r

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.

Citation:

---

Envoyé par transgohan

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

---

Simple :whistle: %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 :koi: pour le signe malgré 1 nombre positif ? pour la sécurité ? 1 peu des 2 ? autre ?

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. :aie:

Citation:

---

Envoyé par transgohan

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.

Citation:

---

Envoyé par transgohan

Un malloc ? ça va pas la tête ? :D
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 ;)

Citation:

---

Envoyé par Bktero

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

---

Non en effet malloc fait la même chose que ce mon code fait, à savoir allouer dans un tableau statique.
Mais quand tu as besoin de compartimenter et/ou classifier la mémoire tu ne peux pas utiliser le malloc de base qui ne pointe que sur un seul espace mémoire. :)

Bon après cela a tout de même ses limites même si cela apporte une certaine résilience face aux développeurs qui ne codent pas les tests de dépassement mémoire (car cela ne va impacter que les données de leur module)...
J'attends toujours l'explication de l'ingénieur en chef indiquant le pourquoi un pointeur fou ne pourrait pas aller corrompre le tableau statique des données de tel module tout comme on pourrait l'avoir avec un seul grand tableau pour malloc...

Tu as peut-être %hhu si ton compilateur le permet ?
Sinon, autant y aller directement avec

Code:

---

1 2 3 4

assert(i < 100); chaine[0] = '0' + i / 10; chaine[1] = '0' + i % 10; chaine[3] = 0;

---

Ça peut potentiellement être plus rapide que le sprintf en plus ? 3 assignations vs appel + parsing du motif + extraction des paramètres pour remplir le motif. :roll:

Merci Bousk je vais tester ce format (que je connaissais pas et que je n'avais pas trouvé avec un man).
Mais j'ai peu d'espoir vis à vis de mon compilateur si même le man de ma distrib n'affiche pas le format alors qu'elle est plus récente. :aie:

Effectivement l'affectation octet par octet est ce que je garde en dernier recours pour palier à ces défauts d'analyse.
Certes cela serait mieux niveau charge CPU mais je ne suis pas dans des portions contraintes en temps donc autant garder la lisibilité au maximum si possible.
J'ai déjà bien assez de portions de code qui nécessite une quantité non négligeable de café et de calme afin de comprendre ce que cela fait, et ce même avec les commentaires pourtant très bien fait.

Tiens ? Moi non plus je ne connaissais pas le "%hh" qui semble être venu du C++ mais qui est accepté par mon gcc.

Citation:

---

Envoyé par Bousk

Sinon, autant y aller directement avec

Code:

---

1 2 3 4

assert(i < 100); chaine[0] = '0' + i / 10; chaine[1] = '0' + i % 10; chaine[3] = 0;

---

Ça peut potentiellement être plus rapide que le sprintf en plus ? 3 assignations vs appel + parsing du motif + extraction des paramètres pour remplir le motif.

---

:applo: :ave:

Citation:

---

Envoyé par transgohan

Effectivement l'affectation octet par octet est ce que je garde en dernier recours pour palier à ces défauts d'analyse.

---

Surtout n'oublie pas le commentaire pour expliquer le but de l'opération. Sinon le prochain qui passe se dira "il est con lui, il ne connait pas sprintf() ?" :mrgreen:

Citation:

---

Envoyé par Sve@r

Surtout n'oublie pas le commentaire pour expliquer le but de l'opération. Sinon le prochain qui passe se dira "il est con lui, il ne connait pas sprintf() ?" :mrgreen:

---

Code:

---

1 2 3 4 5

// Si vous n'avez rien à faire pour les prochains 6 mois mais que vous êtes tout de même financé vous pouvez vous amuser cher développeur à remplacer le code qui suit par un sprintf. // Et passer ainsi un temps infini à chercher une solution de correction du défaut klocwork UNBOUND_SPRINTF sur laquelle j'ai déjà passé un temps non négligeable. // Ou bien passer le temps requis pour rédiger au client le rapport de test ainsi que le code de mise en évidence que c'est un faux positif. // Ou encore vous pouvez passer votre chemin en laissant ce code tel qu'il est et aller vaquer à de meilleurs occupations. // signé : un développeur qui vous veut du bien

---

Citation:

---

Envoyé par transgohan

on en effet malloc fait la même chose que ce mon code fait, à savoir allouer dans un tableau statique.
Mais quand tu as besoin de compartimenter et/ou classifier la mémoire tu ne peux pas utiliser le malloc de base qui ne pointe que sur un seul espace mémoire.

---

Ce qui est intéressant par contre, plutôt que plusieurs allocateurs de "mémoire brute", c'est d'avoir des pools de ressources : au lieu de récupérer des types, du récupère des objets Foo, Bar, Baz, etc. La gestion est simple, il n'y a pas de fragmentation.

Citation:

---

Envoyé par transgohan

J'attends toujours l'explication de l'ingénieur en chef indiquant le pourquoi un pointeur fou ne pourrait pas aller corrompre le tableau statique des données de tel module tout comme on pourrait l'avoir avec un seul grand tableau pour malloc...

---

Beaucoup de gens sont plein de conviction, et parmi eux un nombre non négligeable n'a pas de bonnes explications à ces convictions. Sans voir un peu plus en détails, difficile de juger. Mais par contre, l'allocation statique n'a jamais empêché le jardinage, c'est certain ;)