Al giorno d'oggi molti progetti stanno aprendo il loro codice sorgente e lasciando che coloro che sono interessati allo sviluppo di esso modifichino il codice. Verificheremo uno di questi progetti:OpenJDK e aiuteremo gli sviluppatori a migliorare il codice.
Introduzione
OpenJDK (Open Java Development Kit) – un progetto per la creazione e l'implementazione della piattaforma Java (Java SE), che ora è gratuita e open source. Il progetto è stato avviato nel 2006, dalla società Sun. Il progetto utilizza più linguaggi:C, C++ e Java. Siamo interessati al codice sorgente scritto in C e C++. Prendiamo la nona versione di OpenJDK. Il codice di questa implementazione della piattaforma Java è disponibile nel repository Mercurial.
Il progetto è stato scansionato con l'analizzatore di codice statico PVS-Studio. Ha molte regole diagnostiche, che lo aiutano a trovare un gran numero di errori nel codice ed è anche in grado di trovare quelli difficili da rilevare durante semplici revisioni del codice. Alcuni di questi errori non influiscono sulla logica del programma e alcuni possono portare a tristi conseguenze durante l'esecuzione del programma. Ci sono vari esempi di errori sul sito Web degli analizzatori, che sono stati trovati in altri progetti open source. Questo strumento è in grado di analizzare progetti scritti in C, C++ e C#. La versione di prova dell'analizzatore può essere scaricata a questo link.
Errori nelle espressioni logiche
Per prima cosa diamo un'occhiata agli errori nelle espressioni logiche:
int StubAssembler::call_RT(....) {
#ifdef _LP64
// if there is any conflict use the stack
if (arg1 == c_rarg2 || arg1 == c_rarg3 ||
arg2 == c_rarg1 || arg1 == c_rarg3 ||
arg3 == c_rarg1 || arg1 == c_rarg2) {
....
} Avviso di PVS-Studio: V501 Sono presenti sottoespressioni identiche 'arg1 ==c_rarg3' a sinistra ea destra dell'operatore '||'. c1_Runtime1_x86.cpp 174
L'analizzatore ci parla della duplicazione di arg1 ==c_rarg3 controlla . C'è un controllo ridondante qui, o peggio, un errore logico. Forse dovrebbe essere controllato qualcos'altro invece della condizione duplicata. Questo codice merita sicuramente una revisione.
C'è un'altra espressione ricorrente arg1 ==c_rarg2 :nelle stesse condizioni.
Avviso di PVS-Studio: V501 Sono presenti sottoespressioni identiche 'arg1 ==c_rarg2' a sinistra ea destra dell'operatore '||'. c1_Runtime1_x86.cpp 174
Questi avvisi sono un'ottima prova dell'utilità dell'analizzatore. È molto semplice commettere un errore in un gran numero di espressioni simili; e sono difficili da notare durante la revisione del codice visivo.
Nel frammento successivo abbiamo un controllo "non ideale" nella condizione dell'Ideale metodo:
Node *AddLNode::Ideal(PhaseGVN *phase, bool can_reshape) {
....
if( op2 == Op_AddL &&
in2->in(1) == in1 &&
op1 != Op_ConL &&
0 ) {
....
} Avviso di PVS-Studio: V560 Una parte dell'espressione condizionale è sempre falsa:0. addnode.cpp 435
È abbastanza strano usare 0 in un'espressione logica. Molto probabilmente, questo codice è ancora in fase di sviluppo e, per eseguire il debug, questa condizione non è stata resa eseguibile. Nel codice mancano i commenti necessari e probabilmente in futuro verranno dimenticati. Questo bug può comportare che tutto ciò che si trova all'interno di questa condizione l non venga mai eseguito e, come risultato dell'espressione logica, la valutazione è sempre falsa.
Precedenza delle operazioni
Molto spesso, i programmatori ripongono troppa fiducia nella loro conoscenza della precedenza e non racchiudono le parti componenti tra parentesi di un'espressione complessa:
int method_size() const
{ return sizeof(Method)/wordSize + is_native() ? 2 : 0; } Avviso di PVS-Studio: V502 Forse l'operatore '?:' funziona in modo diverso da come ci si aspettava. L'operatore '?:' ha una priorità inferiore rispetto all'operatore '+'. metodo.hpp 249
In questo caso non conosco le specifiche del codice, ma ho il sospetto che si volesse scegliere un valore '2' o '0' a seconda del risultato della chiamata alla funzione is_native(), ma l'espressione ha un ordine di valutazione diverso. Per prima cosa ci sarà l'aggiunta:sizeof(Method)/wordSize + is_native() , e quindi avremo il risultato 0 o 2 restituito, ovvero il codice doveva essere probabilmente così:
{ return sizeof(Method)/wordSize + (is_native() ? 2 : 0); } Questo è un errore molto comune con la precedenza dell'operazione. Nella base degli errori dell'analizzatore abbiamo trovato i più popolari e li abbiamo inseriti in un articolo:Espressioni logiche in C/C++. Errori commessi da professionisti
Copia-Incolla
Il prossimo gruppo di errori è causato dalla copia del codice. Non c'è modo di aggirare questo metodo preferito dai programmatori, quindi esaminiamo i frammenti in cui lo abbiamo:
static int
setImageHints(....)
{
....
if (dstCMP->isDefaultCompatCM) {
hintP->allocDefaultDst = FALSE;
hintP->cvtToDst = FALSE;
}
else if (dstCMP->isDefaultCompatCM) {
hintP->allocDefaultDst = FALSE;
hintP->cvtToDst = FALSE;
}
....
} Avviso di PVS-Studio: V517 È stato rilevato l'uso del pattern 'if (A) {…} else if (A) {…}'. C'è una probabilità di presenza di un errore logico. Linee di controllo:1873, 1877. awt_ImagingLib.c 1873
In questo frammento le condizioni sono le stesse in if e altrimenti se , così come il codice che dovrebbe essere eseguito. La seconda condizione è del tutto inutile, in quanto non verrà mai eseguita.
Un altro caso simile:
static int expandPackedBCR(JNIEnv *env, RasterS_t *rasterP,
int component,
unsigned char *outDataP)
{
....
/* Convert the all bands */
if (rasterP->numBands < 4) {
/* Need to put in alpha */
for (y=0; y < rasterP->height; y++) {
inP = lineInP;
for (x=0; x < rasterP->width; x++) {
for (c=0; c < rasterP->numBands; c++) {
*outP++ = (unsigned char)
(((*inP&rasterP->sppsm.maskArray[c]) >> roff[c])
<<loff[c]);
}
inP++;
}
lineInP += rasterP->scanlineStride;
}
}
else {
for (y=0; y < rasterP->height; y++) {
inP = lineInP;
for (x=0; x < rasterP->width; x++) {
for (c=0; c < rasterP->numBands; c++) {
*outP++ = (unsigned char)
(((*inP&rasterP->sppsm.maskArray[c]) >> roff[c])
<<loff[c]);
}
inP++;
}
lineInP += rasterP->scanlineStride;
}
}
....
} Avviso di PVS-Studio: V523 L'istruzione 'allora' è equivalente all'istruzione 'else'. awt_ImagingLib.c 2927
Il codice eseguibile è identico in entrambi i blocchi, quindi non c'è differenza in ciò che viene valutato nella condizione. Ha senso guardare questo frammento e rimuovere il ramo non necessario o correggere il codice se si prevedeva una logica diversa, per evitare duplicazioni.
Altri due frammenti con duplicazione identica. Li mostrerò qui senza citare il codice:
- V523 L'istruzione 'then' è equivalente all'istruzione 'else'. awt_ImagingLib.c 3111
- V523 L'istruzione 'then' è equivalente all'istruzione 'else'. awt_ImagingLib.c 3307
E l'ultimo caso interessante, causato dall'errore di copia incolla:
Node* GraphKit::record_profiled_receiver_for_speculation(Node* n)
{
....
ciKlass* exact_kls = profile_has_unique_klass();
bool maybe_null = true;
if (java_bc() == Bytecodes::_checkcast ||
java_bc() == Bytecodes::_instanceof ||
java_bc() == Bytecodes::_aastore) {
ciProfileData* data =
method()->method_data()->bci_to_data(bci());
bool maybe_null = data == NULL ? true : <==
data->as_BitData()->null_seen();
}
return record_profile_for_speculation(n,
exact_kls, maybe_null);
return n;
} Avviso di PVS-Studio: V561 Probabilmente è meglio assegnare un valore alla variabile "maybe_null" piuttosto che dichiararla di nuovo. Dichiarazione precedente:graphKit.cpp, riga 2170. graphKit.cpp 2175
Cosa sta succedendo in questo codice? Una variabile bool may_null =true; viene dichiarato prima del blocco if. Quindi, quando viene eseguito il codice in if clock, viene dichiarata una variabile con lo stesso nome. Dopo l'uscita dal blocco, il valore di questa variabile andrà perso e la chiamata alla funzione, utilizzando questa variabile, sarà sempre vera. Va bene se la variabile è stata duplicata per motivi di debug. In caso contrario, questo codice viene eseguito in modo errato e richiede la modifica:
maybe_null = data == NULL ? true :
data->as_BitData()->null_seen(); Gestione del puntatore
Un programmatore dovrebbe essere molto attento, e particolarmente attento, quando lavora con i puntatori; perché durante l'utilizzo del puntatore potresti ricevere errori che saranno difficili da rilevare in seguito. Di norma, il pericolo principale consiste nell'utilizzare puntatori non validi o nell'utilizzare puntatori senza verificarli rispetto a null.
In primo luogo, diamo un'occhiata a un caso di uso esplicito di un puntatore nullo:
static jint JNICALL
cbObjectTagInstance(....)
{
ClassInstancesData *data;
/* Check data structure */
data = (ClassInstancesData*)user_data;
if (data == NULL) {
data->error = AGENT_ERROR_ILLEGAL_ARGUMENT;
return JVMTI_VISIT_ABORT;
}
....
} Avviso di PVS-Studio: V522 Potrebbe aver luogo il dereferenziamento dei "dati" del puntatore nullo. util.c 2424
Un codice completamente poco chiaro con un puntatore nullo può causare un arresto anomalo del programma. Forse questo ramo non è mai stato eseguito, ecco perché alcuni problemi sono stati evitati. C'erano altri tre frammenti simili nello stesso file:
- V522 Potrebbe aver luogo il dereferenziamento dei "dati" del puntatore nullo. util.c 2543
- V522 Potrebbe aver luogo il dereferenziamento dei "dati" del puntatore nullo. util.c 2601
- V522 Potrebbe aver luogo il dereferenziamento dei "dati" del puntatore nullo. util.c 2760
Ma nei seguenti casi la possibilità di utilizzare un puntatore nullo non è così evidente. Questa è una situazione molto comune, tali avvisi si trovano in quasi tutti i progetti che controlliamo.
static jboolean
visibleClasses(PacketInputStream *in, PacketOutputStream *out)
{
....
else {
(void)outStream_writeInt(out, count);
for (i = 0; i < count; i++) {
jbyte tag;
jclass clazz;
clazz = classes[i]; <==
tag = referenceTypeTag(clazz);
(void)outStream_writeByte(out, tag);
(void)outStream_writeObjectRef(env, out, clazz);
}
}
if ( classes != NULL ) <==
jvmtiDeallocate(classes);
....
return JNI_TRUE;
}
Avviso di PVS-Studio :V595 Il puntatore 'classi' è stato utilizzato prima di essere verificato rispetto a nullptr. Righe di controllo:58, 66. ClassLoaderReferenceImpl.c 58
Nel blocco inferiore il puntatore viene verificato rispetto a null, quindi il programmatore presume che un valore del puntatore possa essere nullo. Ma nel blocco sopra vediamo che il puntatore viene utilizzato senza un segno di spunta. Pertanto, se il valore del puntatore è zero, questo controllo non ci aiuterà e il programma terminerà. Per correggere questo errore, dovremmo controllare il puntatore che si trova sopra i due blocchi.
Faccio un esempio simile:
int InstructForm::needs_base_oop_edge(FormDict &globals) const {
if( is_simple_chain_rule(globals) ) {
const char *src = _matrule->_rChild->_opType;
OperandForm *src_op = globals[src]->is_operand();
assert( src_op, "Not operand class of chain rule" );
return src_op->_matrule ?
src_op->_matrule->needs_base_oop_edge() : 0;
} // Else check instruction
return _matrule ? _matrule->needs_base_oop_edge() : 0;
} Avviso di PVS-Studio: V595 Il puntatore '_matrule' è stato utilizzato prima che fosse verificato rispetto a nullptr. Linee di controllo:3534, 3540. formssel.cpp 3534
Qui il controllo del puntatore viene eseguito di seguito nell'operatore ternario – _matrule ? _matrule->bisogno_base_oop_edge() :0;. In precedenza nel codice c'è l'indirizzamento del puntatore – const char *src =_matrule->_rChild->_opType;. La ricetta per correggerlo è la stessa:il puntatore va controllato prima di utilizzarlo. C'erano parecchi di questi posti, quindi li darò come elenco qui:
- V595 Il puntatore '_pipeline' è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:3265, 3274. output_c.cpp 3265
- V595 Il puntatore 'index_bound' è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:790, 806. c1_RangeCheckElimination.cpp 790
- V595 Il puntatore 'g_type_init' è stato utilizzato prima che fosse verificato rispetto a nullptr. Linee di controllo:94, 108. GioFileTypeDetector.c 94
- V595 Il puntatore 'classArray' è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:1169, 1185. JPLISAgent.c 1169
- V595 Il puntatore 'q' è stato utilizzato prima di essere verificato rispetto a nullptr. Righe di controllo:594, 599. mpi.c 594
- V595 Il puntatore "info.waiters" è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:224, 228. ObjectReferenceImpl.c 224
- V595 Il puntatore "metodi" è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:225, 229. ReferenceTypeImpl.c 225
- V595 Il puntatore 'campi' è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:433, 437. ReferenceTypeImpl.c 433
- V595 Il puntatore "nidificato" è stato utilizzato prima di essere verificato rispetto a nullptr. Righe di controllo:538, 540. ReferenceTypeImpl.c 538
- V595 Il puntatore 'interfacce' è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:593, 595. ReferenceTypeImpl.c 593
- V595 Il puntatore 'buf' è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:265, 266. ps_proc.c 265
- V595 Il puntatore "monitor" è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:382, 387. ThreadReferenceImpl.c 382
- V595 Il puntatore "monitor" è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:557, 560. ThreadReferenceImpl.c 557
- V595 Il puntatore 'firma' è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:520, 526. debugInit.c 520
- V595 Il puntatore 'BlackPoint' è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:192, 208. cmssamp.c 192
- V595 Il puntatore 'nativename' è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:506, 511. awt_Font.c 506
- V595 Il puntatore 'pseq->seq' è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:788, 791. cmsnamed.c 788
- V595 Il puntatore 'GammaTables' è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo:1430, 1434. cmsopt.c 1430
A volte i programmatori controllano i puntatori, ma lo fanno in modo sbagliato.
FileBuff::FileBuff( BufferedFile *fptr, ArchDesc& archDesc) :
_fp(fptr), _AD(archDesc) {
....
_bigbuf = new char[_bufferSize];
if( !_bigbuf ) {
file_error(SEMERR, 0, "Buffer allocation failed\n");
exit(1);
....
} Avviso di PVS-Studio: V668 Non ha senso testare il puntatore '_bigbuf' rispetto a null, poiché la memoria è stata allocata utilizzando l'operatore 'new'. L'eccezione verrà generata in caso di errore di allocazione della memoria. filebuff.cpp 47
In questo caso la verifica della verifica di _bigbuf il puntatore contro null dopo aver usato l'operatore new è inutile. Nel caso in cui il sistema non sia in grado di allocare la memoria, verrà generata un'eccezione e l'esecuzione della funzione verrà interrotta. Possiamo utilizzare diversi approcci per risolvere questo problema. Potremmo allocare la memoria nel try catch bloccare o utilizzare new(std::nothrow) costruzione, che non genererà eccezioni in caso di errore. Ci sono molti altri controlli errati.
- V668 Non ha senso testare il puntatore "vspace" rispetto a null, poiché la memoria è stata allocata utilizzando l'operatore "new". L'eccezione verrà generata in caso di errore di allocazione della memoria. psParallelCompact.cpp 455
- V668 Non ha senso testare il puntatore 'uPtr' rispetto a null, poiché la memoria è stata allocata utilizzando l'operatore 'new'. L'eccezione verrà generata in caso di errore di allocazione della memoria. jni.cpp 113
L'ultimo errore relativo alla gestione del puntatore si è verificato durante il cast esplicito di un tipo di puntatore a un altro.
mlib_status mlib_convMxNext_f32(...)
{
mlib_d64 dspace[1024], *dsa = dspace;
....
mlib_f32 *fsa;
....
if (3 * wid_e + m > 1024) {
dsa = mlib_malloc((3 * wid_e + m) * sizeof(mlib_d64));
if (dsa == NULL)
return MLIB_FAILURE;
}
fsa = (mlib_f32 *) dsa; <==
....
} Avviso di PVS-Studio :V615 Una strana conversione esplicita dal tipo 'double *' al tipo 'float *'. mlib_ImageConvMxN_Fp.c 294
Un programmatore prova ad assegnare un puntatore a float mlib_f32 *fsa con un puntatore mlib_d64 dspace[1024], *dsa =dspace . Ma i tipi float e double hanno dimensioni diverse, quindi questo tipo di cast è molto probabilmente errato. La mancata corrispondenza dei tipi cast provoca il fsa t o puntare a una cifra che non è corretta per il virgola mobile digita .
Ci sono altri due casting simili in un altro file, sarebbe una buona cosa controllare questo codice e utilizzare i casting di tipo corretti.
- V615 Una conversione esplicita dispari dal tipo 'double *' al tipo 'float *'. mlib_ImageLookUp_Bit.c 525
- V615 Una conversione esplicita dispari dal tipo 'double *' al tipo 'float *'. mlib_ImageLookUp_Bit.c 526
A questo punto, smettiamo di guardare agli errori relativi alla gestione errata del puntatore e passiamo agli altri avvisi dell'analizzatore.
Errori vari
Il seguente bug è probabilmente il risultato di un copia-incolla improprio:
static bool
parse_bool (const char **pp, const char *end, unsigned int *pv)
{
....
/* CSS allows on/off as aliases 1/0. */
if (*pp - p == 2 || 0 == strncmp (p, "on", 2))
*pv = 1;
else if (*pp - p == 3 || 0 == strncmp (p, "off", 2))
*pv = 0;
else
return false;
return true;
} Avviso di PVS-Studio: V666 Considerare di esaminare il terzo argomento della funzione 'strncmp'. È possibile che il valore non corrisponda alla lunghezza di una stringa passata con il secondo argomento. hb-shape.cc 104
Ecco un caso in cui un bug non influisce sul funzionamento del programma. Invece di confrontare tre simboli, vengono confrontati solo i primi due simboli, ma non concludo che l'autore del codice non abbia eseguito questo controllo deliberatamente. Poiché il valore nel buffer p può essere attivato o disattivato, è sufficiente confrontare i primi due simboli. Ma per rendere più chiaro, possiamo correggere il codice:
else if (*pp - p == 3 || 0 == strncmp (p, "off", 3))
C'erano molti altri posti
class ProductionState {
....
private:
// Disable public use of constructor, copy-ctor, ...
ProductionState( ) :
_production(cmpstr, hashstr, Form::arena)
{ assert( false, "NotImplemented"); };
ProductionState( const ProductionState & ) :
_production(cmpstr, hashstr, Form::arena)
{ assert( false, "NotImplemented"); }; // Deep-copy
}; Avviso di PVS-Studio: Il costruttore di copia V690 è dichiarato privato nella classe "ProductionState", ma l'operatore "=" predefinito verrà comunque generato dal compilatore. È pericoloso usare una classe del genere. dfa.cpp 76
In questa classe il programmatore ha tentato di vietare la copia del codice, ma ha dimenticato di aggiungere un operatore di assegnazione della copia in un'area privata. Verrà generato per impostazione predefinita e sarà disponibile per l'uso. Anche se questo operatore non viene utilizzato da nessuna parte nel codice, c'è la garanzia che non verrà chiamato accidentalmente in futuro. Durante la chiamata di un tale operatore avremo la copia a livello di membro per una classe che non dovrebbe essere copiata. Ciò può causare vari effetti, persino un arresto anomalo del programma. In questo caso dobbiamo aggiungere all'area riservata la dichiarazione dell'operatore “=”.
Ci sono altre due classi in cui vediamo gli stessi problemi; sarebbe fantastico sistemarli in modo tale che la "Legge dei due grandi" non venga violata.
- V690 La classe 'MemRegion' implementa un costruttore di copia, ma manca dell'operatore '='. È pericoloso usare una classe del genere. memRegion.hpp 43
- Il costruttore di copia V690 è dichiarato privato nella classe 'Label', ma l'operatore '=' predefinito verrà comunque generato dal compilatore. È pericoloso usare una classe del genere. assembler.hpp 73
Quest'ultimo sembra un semplice errore di battitura.
bool os::start_debugging(char *buf, int buflen) {
int len = (int)strlen(buf);
char *p = &buf[len];
....
if (yes) {
// yes, user asked VM to launch debugger
jio_snprintf(buf, sizeof(buf), "gdb /proc/%d/exe %d",
os::current_process_id(), os::current_process_id());
os::fork_and_exec(buf);
yes = false;
}
return yes;
} Avviso di PVS-Studio: V579 La funzione jio_snprintf riceve il puntatore e la sua dimensione come argomenti. Forse è un errore. Esamina il secondo argomento. os_linux.cpp 6094
Un programmatore voleva passare una dimensione del buffer, ma non ha tenuto conto del fatto che non è un array dichiarato localmente, ma un puntatore che arriva nell'argomento della funzione. Nel risultato della valutazione di sizeof(buf) non otterremo la dimensione del buffer, ma la dimensione del puntatore, che sarà di 4 o 8 byte. Questo bug può essere risolto facilmente, poiché la lunghezza del buffer era già stata ricevuta in precedenza nel codice:int len =(int)strlen(buf);. La variante corretta sarà la seguente:
jio_snprintf(buf, len ....
Conclusione
È sempre divertente controllare un progetto che viene utilizzato e mantenuto da un gran numero di persone. Abbiamo riscontrato un numero considerevole di errori; in questo articolo ne abbiamo descritto solo una parte, il resto richiede un'indagine più approfondita. Quei bug che abbiamo riscontrato sono un'ulteriore prova dell'utilità di un analizzatore, in quanto consente il rilevamento di errori che altrimenti sarebbero difficili da rilevare durante la semplice revisione del codice. Il modo più efficace è utilizzare un analizzatore su base regolare, poiché farà risparmiare molto tempo che potrebbe essere speso per il debug del programma. E ancora, ti ricordo, che puoi provare l'analizzatore sul tuo progetto scaricando la versione di prova.
Di Svyatoslav Razmyslov