Noi, gli sviluppatori di analizzatori di codici statici di PVS-Studio, abbiamo una visione particolare della bellezza. Sulla bellezza degli insetti. Ci piace trovare grazia negli errori, esaminarli, cercare di indovinare come sono apparsi. Oggi abbiamo un caso interessante in cui i concetti di lunghezza e dimensione sono stati confusi nel codice.
Errore del progetto LFortran
Quando abbiamo saputo del nuovo numero di CppCast su LFortran, abbiamo deciso di controllare proprio questo LFortran. Questo è un piccolo progetto, quindi non sappiamo se ci sarà abbastanza materiale per un classico articolo sull'analisi dei progetti open source. Tuttavia, un piccolo errore ha subito attirato la nostra attenzione, quindi abbiamo deciso di scrivere una piccola nota. A nostro gusto, questo è un errore adorabile.
Il progetto LFortran ha funzioni che concatenano due stringhe in un nuovo buffer.
void _lfortran_strcat(char** s1, char** s2, char** dest)
{
int cntr = 0;
char trmn = '\0';
int s1_len = strlen(*s1);
int s2_len = strlen(*s2);
int trmn_size = strlen(&trmn);
char* dest_char = (char*)malloc(s1_len+s2_len+trmn_size);
for (int i = 0; i < s1_len; i++) {
dest_char[cntr] = (*s1)[i];
cntr++;
}
for (int i = 0; i < s2_len; i++) {
dest_char[cntr] = (*s2)[i];
cntr++;
}
dest_char[cntr] = trmn;
*dest = &(dest_char[0]);
}Prima di analizzare questo codice, puoi provare a trovare tu stesso un errore. Inserisco una foto lunga in modo da non leggere per sbaglio la spiegazione. Probabilmente hai visto il meme "gatto lungo". Avremo un "unicorno lungo" :)
La funzione dovrebbe funzionare nel modo seguente. Calcoliamo una dimensione del buffer che può contenere sia le stringhe unite, sia il terminale null. Il buffer è allocato, copiamo le stringhe al suo interno e aggiungiamo il terminale null. Tuttavia, il buffer allocato ha una dimensione insufficiente. La sua dimensione è 1 byte in meno di quanto richiesto. Di conseguenza, il terminale null verrà scritto al di fuori del buffer allocato.
Lo sviluppatore che ha scritto il codice si è lasciato trasportare dall'uso eccessivo di strlen funzione. L'autore lo ha persino utilizzato per determinare la dimensione nulla del terminale. C'era una confusione tra la dimensione di un oggetto (terminale null) e la lunghezza di una stringa vuota. Questo codice è strano e errato. Ma per noi è un errore bellissimo e insolito.
Spiegazione:
char trmn = '\0';
int trmn_size = strlen(&trmn);Qui, il trmn Il simbolo viene interpretato come una stringa vuota la cui lunghezza è zero. Di conseguenza, il trmn_size variabile, il cui nome sta per la dimensione nulla del terminale, è sempre uguale a 0.
Non avrebbero dovuto contare la lunghezza della stringa vuota. È meglio calcolare quanti byte occupa il carattere del terminale con la sizeof operatore. Il codice corretto:
void _lfortran_strcat(char** s1, char** s2, char** dest)
{
int cntr = 0;
char trmn = '\0';
int s1_len = strlen(*s1);
int s2_len = strlen(*s2);
int trmn_size = sizeof(trmn); // <=
char* dest_char = (char*)malloc(s1_len+s2_len+trmn_size);
for (int i = 0; i < s1_len; i++) {
dest_char[cntr] = (*s1)[i];
cntr++;
}
for (int i = 0; i < s2_len; i++) {
dest_char[cntr] = (*s2)[i];
cntr++;
}
dest_char[cntr] = trmn;
*dest = &(dest_char[0]);
}Rilevamento errori
Abbiamo trovato l'errore con l'analizzatore di codice statico PVS-Studio. Sfortunatamente, lo strumento non è stato in grado di rilevare l'errore come indice di matrice fuori limite. Questo è piuttosto difficile da fare. L'analisi del flusso di dati non ha potuto confrontare come la dimensione del dest_char buffer è correlato a cntr valore della variabile che viene incrementato nel ciclo. L'errore è stato rilevato indirettamente.
PVS-Studio ha emesso un avviso:V742 [CWE-170, CERT-EXP37-C] La funzione riceve un indirizzo di una variabile di tipo 'char' invece del puntatore a un buffer. Esamina il primo argomento. lfortran_intrinsics.c 550
È strano calcolare la lunghezza di una stringa con strlen funzione passando un puntatore a un singolo simbolo a questa funzione. Infatti, quando abbiamo esaminato l'anomalia, abbiamo riscontrato un bug grave. L'analisi statica è fantastica!
Continuiamo a migliorare il codice
Abbiamo corretto l'errore. Tuttavia, il codice presenta altri inconvenienti che l'analizzatore ha evidenziato. Sarebbe utile fare un ulteriore refactoring.
Innanzitutto, all'analizzatore non piace la mancanza di un controllo aggiuntivo di un puntatore che il malloc la funzione ritorna. Questo è importante. Avvertenza:V522 [CWE-690, CERT-MEM52-CPP] Potrebbe esserci il dereferenziamento di un potenziale puntatore nullo 'dest_char'. Righe di controllo:553, 551. lfortran_intrinsics.c 553
In secondo luogo, l'analizzatore emette diversi avvisi sugli errori a 64 bit. Il codice non è preparato per stringhe che possono essere più lunghe di INT_MAX personaggi. Questo è chiaramente esotico, ma scrivere codice in questo modo è comunque brutto e potenzialmente pericoloso. È meglio usare size_t digita invece di int .
La versione migliorata della funzione:
void _lfortran_strcat(const char** s1, const char** s2, char** dest)
{
if (s1 == NULL || *s1 == NULL ||
s2 == NULL || *s2 == NULL || dest == NULL)
{
// Some kind of error handling appropriate in the given project.
....
}
size_t cntr = 0;
const char trmn = '\0';
const size_t s1_len = strlen(*s1);
const size_t s2_len = strlen(*s2);
char* dest_char = (char*)malloc((s1_len+s2_len+1)*sizeof(char));
if (dest_char == NULL)
{
// Some kind of error handling appropriate in the given project.
....
}
for (size_t i = 0; i < s1_len; i++) {
dest_char[cntr] = (*s1)[i];
cntr++;
}
for (size_t i = 0; i < s2_len; i++) {
dest_char[cntr] = (*s2)[i];
cntr++;
}
dest_char[cntr] = trmn;
*dest = dest_char;
}Anche il nuovo codice non è perfetto, ma è chiaramente migliorato. Grazie per l'attenzione. Vieni a provare PVS-Studio per testare i tuoi progetti.
Link aggiuntivi:
- Avvio della mia raccolta di bug trovati nelle funzioni di copia
- PVS-Studio impara cos'è lo strlen
- Lezioni sullo sviluppo di applicazioni a 64 bit