A.
El desarrollo de grandes proyectos complejos es imposible sin el uso de metodologías de programación y herramientas para ayudar a controlar la calidad del código. En primer lugar, es un estándar de codificación competente, revisiones de código, pruebas unitarias, analizadores de código estático y dinámico. Todo esto ayuda a identificar defectos en el código en las primeras etapas de desarrollo. Este artículo demuestra las capacidades del analizador estático PVS-Studio para detectar errores y vulnerabilidades potenciales en el código del sistema operativo Android. Esperamos que el artículo atraiga a los lectores a la metodología del análisis de código estático y quieran implementarlo en el proceso de desarrollo de sus propios proyectos.
Introduccion
Ha pasado un año desde la redacción de un gran
artículo sobre errores en el sistema operativo Tizen, y quería volver a realizar un estudio igualmente interesante de algún tipo de sistema operativo. La elección recayó en Android.
El código del sistema operativo Android es de calidad, bien probado. Durante el desarrollo, se utiliza al menos un analizador de código estático Coverity, como lo demuestran los comentarios de la forma:
En general, este es un proyecto interesante y de alta calidad, y encontrar errores en él es un desafío para nuestro analizador estático PVS-Studio.
Creo que solo por el volumen del artículo, el lector comprende que el analizador PVS-Studio hizo un excelente trabajo y encontró muchos defectos en el código de este sistema operativo.
Enumeración de debilidad común
En el artículo encontrará referencias a la
enumeración de debilidad común (CWE). Permítanos explicar la razón para referirse a esta lista y por qué es importante desde el punto de vista de la seguridad.
A menudo, la causa de las vulnerabilidades en los programas no es un conjunto complicado de circunstancias, sino un error de software banal. Aquí será apropiado citar esta cita de
prqa.com :
El Instituto Nacional de Estándares y Tecnología (NIST) informa que el 64% de las vulnerabilidades de software provienen de errores de programación y no de la falta de características de seguridad.
Puede leer en el artículo "
¿Cómo PVS-Studio puede ayudar a encontrar vulnerabilidades? " Con algunos ejemplos de errores simples que condujeron a vulnerabilidades en proyectos como MySQL, iOS, NAS, illumos-gate.
En consecuencia, se pueden eliminar muchas vulnerabilidades al detectar errores comunes a tiempo y corregirlos. Y aquí la Enumeración de Debilidad Común entra en escena.
Los errores son diferentes, y no todos los errores son peligrosos desde el punto de vista de la seguridad. Los errores que podrían causar vulnerabilidades se recopilan en la enumeración de debilidad común. Esta lista se está actualizando y probablemente haya errores que pueden generar vulnerabilidades, pero aún no se han incluido en esta lista.
Sin embargo, si el error se clasifica según el CWE, significa que es teóricamente posible que pueda explotarse como una vulnerabilidad (
CVE ). Sí, la probabilidad de esto es pequeña. Muy raramente, CWE se convierte en CVE. Sin embargo, si desea proteger su código de vulnerabilidades, debe, si es posible, encontrar tantos errores descritos en CWE como sea posible y solucionarlos.
Esquemáticamente, la relación entre PVS-Studio, errores, CWE y CVE se muestra en la figura:
Algunos errores se clasifican como CWE. PVS-Studio puede detectar muchos de estos errores, evitando así que algunos de estos defectos se conviertan en vulnerabilidades (CVE).
Podemos decir que PVS-Studio identifica muchas vulnerabilidades potenciales antes de que causen daños. Por lo tanto, PVS-Studio es una herramienta de prueba de seguridad de aplicaciones estáticas (
SAST ).
Ahora, creo, está claro por qué, al describir los errores, consideré importante tener en cuenta cómo se clasifican según el CWE. De esta manera, es más fácil mostrar la importancia de usar un analizador de código estático en proyectos críticos, con los que los sistemas operativos se relacionan claramente.
Verificación de Android
Para analizar el proyecto, utilicé el analizador PVS-Studio versión 6.24. El analizador actualmente admite los siguientes lenguajes y compiladores:
- Ventanas Visual Studio 2010-2017 C, C ++, C ++ / CLI, C ++ / CX (WinRT), C #
- Ventanas IAR Embedded Workbench, compilador C / C ++ para ARM C, C ++
- Windows / Linux Keil µVision, DS-MDK, compilador ARM 5/6 C, C ++
- Windows / Linux Texas Instruments Code Composer Studio, Herramientas de generación de código ARM C, C ++
- Windows / Linux / macOS. Clang C, C ++
- Linux / macOS. CCG C, C ++
- Ventanas MinGW C, C ++
Nota Quizás algunos de nuestros lectores se hayan perdido la noticia de que apoyamos el trabajo en el entorno macOS y les interesará esta publicación: "
Versión PVS-Studio para macOS: 64 debilidades en el núcleo Apple XNU ".
El proceso de verificar el código fuente de Android no fue un problema, por lo que no me detendré en él. El problema, más bien, era mi preocupación por otras tareas, por lo que no encontré el tiempo y la energía para mirar el informe tan cuidadosamente como me gustaría. Sin embargo, incluso un vistazo rápido fue más que suficiente para reunir una gran colección de errores interesantes para un artículo sólido.
Lo más importante: les pido a los desarrolladores de Android que no solo corrijan los errores descritos en el artículo, sino que también realicen un análisis independiente más cuidadoso. Miré el informe del analizador superficialmente y podría haber pasado por alto muchos errores graves.
En la primera prueba, el analizador producirá muchos falsos positivos, pero
esto no es un problema . Nuestro equipo está listo para ayudar con las recomendaciones sobre la configuración del analizador para reducir la cantidad de falsos positivos. También estamos listos para proporcionar una clave de licencia por un mes o más, si es necesario. En general,
escríbenos , te ayudaremos y te lo diremos.
Ahora veamos qué errores y vulnerabilidades potenciales logré encontrar. Espero que disfrute lo que puede encontrar el analizador de código estático PVS-Studio. Que tengas una buena lectura.
Comparaciones sin sentido
El analizador considera que las expresiones son anormales si siempre son verdaderas o falsas. Dichas advertencias, de acuerdo con la enumeración de debilidad común, se clasifican como:
- CWE-570 : La expresión siempre es falsa
- CWE-571 : La expresión es siempre verdadera
El analizador genera muchas de estas advertencias y, desafortunadamente, la mayoría de ellas son falsas para el código de Android. En este caso, el analizador no tiene la culpa. Solo el código está escrito así.
Lo demostraré con un simple ejemplo.
#if GENERIC_TARGET const char alternative_config_path[] = "/data/nfc/"; #else const char alternative_config_path[] = ""; #endif CNxpNfcConfig& CNxpNfcConfig::GetInstance() { .... if (alternative_config_path[0] != '\0') { .... }
Aquí el analizador genera una advertencia: V547 CWE-570 La expresión 'alternative_config_path [0]! =' \ 0 '' siempre es falsa. phNxpConfig.cpp 401
El hecho es que la macro
GENERIC_TARGET no está definida y, desde el punto de vista del analizador, el código se ve así:
const char alternative_config_path[] = ""; .... if (alternative_config_path[0] != '\0') {
El analizador simplemente está obligado a emitir una advertencia, ya que la línea está vacía y un terminal cero siempre se encuentra en el desplazamiento cero. Por lo tanto, el analizador tiene formalmente razón al emitir una advertencia. Sin embargo, desde un punto de vista práctico, no hay beneficio de esta advertencia.
Desafortunadamente, nada se puede hacer con tales situaciones. Tendremos que revisar sistemáticamente todas esas advertencias y marcar muchos lugares como falsos positivos para que el analizador ya no emita mensajes en estas líneas. Esto realmente debe hacerse porque, además de los mensajes sin sentido, se encontrarán muchos defectos reales.
Admito honestamente que no estaba interesado en mirar cuidadosamente las advertencias de este tipo, y las revisé superficialmente. Sin embargo, incluso esto será suficiente para mostrar que tales diagnósticos son muy útiles y encuentran errores interesantes.
Quiero comenzar con la situación clásica cuando la función de comparar dos objetos se implementa incorrectamente. ¿Por qué clásico? Este es un patrón de error típico que encontramos constantemente en una variedad de proyectos. Lo más probable es que haya tres razones para que ocurra:
- Las funciones de comparación son simples y están escritas "automáticamente" y utilizando la tecnología Copy-Paste. Al escribir dicho código, una persona no está atenta y a menudo comete errores tipográficos.
- Por lo general, la revisión de código no se realiza para tales funciones, ya que es demasiado vago para considerar funciones simples y aburridas.
- Las pruebas unitarias generalmente no se realizan para tales funciones. Porque la pereza. Además, las funciones son simples y los programadores no piensan que sean posibles errores en ellas.
Estos pensamientos y ejemplos se describen con más detalle en el artículo "El
mal vive en las funciones de comparación ".
static inline bool isAudioPlaybackRateEqual( const AudioPlaybackRate &pr1, const AudioPlaybackRate &pr2) { return fabs(pr1.mSpeed - pr2.mSpeed) < AUDIO_TIMESTRETCH_SPEED_MIN_DELTA && fabs(pr1.mPitch - pr2.mPitch) < AUDIO_TIMESTRETCH_PITCH_MIN_DELTA && pr2.mStretchMode == pr2.mStretchMode && pr2.mFallbackMode == pr2.mFallbackMode; }
Entonces, ante nosotros está la función clásica de comparar dos objetos de tipo
AudioPlaybackRate . Y, como creo, el lector adivina que está mal. El analizador PVS-Studio nota dos errores tipográficos de inmediato:
- V501 CWE-571 Hay subexpresiones idénticas a la izquierda y a la derecha del operador '==': pr2.mStretchMode == pr2.mStretchMode AudioResamplerPublic.h 107
- V501 CWE-571 Hay subexpresiones idénticas a la izquierda y a la derecha del operador '==': pr2.mFallbackMode == pr2.mFallbackMode AudioResamplerPublic.h 108
El campo
pr2.mStretchMode y el campo
pr2.mFallbackMode se comparan entre sí. Resulta que la función no compara objetos con la suficiente precisión.
La siguiente comparación sin sentido vive en una función que almacena información de huellas digitales en un archivo.
static void saveFingerprint(worker_thread_t* listener, int idx) { .... int ns = fwrite(&listener->secureid[idx], sizeof(uint64_t), 1, fp); .... int nf = fwrite(&listener->fingerid[idx], sizeof(uint64_t), 1, fp); if (ns != 1 || ns !=1)
Dos diagnósticos detectan una anomalía en este código a la vez:
- V501 CWE-570 Hay subexpresiones idénticas a la izquierda y a la derecha de '||' operador: ns! = 1 || ns! = 1 huella digital.c 126
- V560 CWE-570 Una parte de la expresión condicional siempre es falsa: ns! = 1. fingerprint.c 126
No se maneja la situación cuando la segunda llamada a la función
fwrite no puede escribir datos en un archivo. En otras palabras, el valor de la variable
nf no
está marcado. La verificación correcta debería verse así:
if (ns != 1 || nf != 1)
Pasamos al siguiente error asociado con el uso del operador
& .
#define O_RDONLY 00000000 #define O_WRONLY 00000001 #define O_RDWR 00000002 static ssize_t verity_read(fec_handle *f, ....) { .... if (f->mode & O_RDONLY && expect_zeros) { memset(data, 0, FEC_BLOCKSIZE); goto valid; } .... }
Advertencia de PVS-Studio: V560 CWE-570 Una parte de la expresión condicional siempre es falsa: f-> mode & 00000000. fec_read.cpp 322
Tenga en cuenta que la constante
O_RDONLY es cero. Esto hace que la expresión
f-> mode & O_RDONLY no tenga sentido, ya que siempre es 0. Resulta que la condición de la
declaración if nunca se cumple, y la declaración-true es un código muerto.
La verificación correcta debería ser así:
if (f->mode == O_RDONLY && expect_zeros) {
Ahora veamos un error tipográfico clásico cuando olvidamos escribir parte de la condición.
enum { .... CHANGE_DISPLAY_INFO = 1 << 2, .... }; void RotaryEncoderInputMapper::configure(nsecs_t when, const InputReaderConfiguration* config, uint32_t changes) { .... if (!changes || (InputReaderConfiguration::CHANGE_DISPLAY_INFO)) { .... }
Advertencia de PVS-Studio: V768 CWE-571 La constante de enumeración 'CHANGE_DISPLAY_INFO' se usa como una variable de tipo booleano. InputReader.cpp 3016
La condición siempre es verdadera, ya que el operando
InputReaderConfiguration :: CHANGE_DISPLAY_INFO es una constante igual a 4.
Si observa cómo se escribe el código en el vecindario, queda claro que, de hecho, la condición debería ser así:
if (!changes || (changes & InputReaderConfiguration::CHANGE_DISPLAY_INFO)) {
La siguiente comparación, que no tiene sentido, la conocí en el operador de bucle.
void parse_printerAttributes(....) { .... ipp_t *collection = ippGetCollection(attrptr, i); for (j = 0, attrptr = ippFirstAttribute(collection); (j < 4) && (attrptr != NULL); attrptr = ippNextAttribute(collection)) { if (strcmp("....", ippGetName(attrptr)) == 0) { ....TopMargin = ippGetInteger(attrptr, 0); } else if (strcmp("....", ippGetName(attrptr)) == 0) { ....BottomMargin = ippGetInteger(attrptr, 0); } else if (strcmp("....", ippGetName(attrptr)) == 0) { ....LeftMargin = ippGetInteger(attrptr, 0); } else if (strcmp("....", ippGetName(attrptr)) == 0) { ....RightMargin = ippGetInteger(attrptr, 0); } } .... }
Advertencia de PVS-Studio: V560 CWE-571 Una parte de la expresión condicional siempre es verdadera: (j <4). ipphelper.c 926
Tenga en cuenta que el valor de la variable
j no se incrementa en ninguna parte. Esto significa que la subexpresión
(j <4) siempre
es verdadera.
El mayor número de operaciones útiles del analizador PVS-Studio, en relación con las condiciones siempre verdaderas / falsas, se refiere al código donde se verifica el resultado de crear objetos utilizando el
nuevo operador. En otras palabras, el analizador detecta el siguiente patrón de código:
T *p = new T; if (p == nullptr) return ERROR;
Tales controles no tienen sentido. Si no fue posible asignar memoria para el objeto, se
genera una excepción del tipo
std :: bad_alloc, y simplemente no llegará a la comprobación del valor del puntero.
Nota El
nuevo operador puede devolver
nullptr escribiendo
new (std :: nothrow) T. Sin embargo, esto no se aplica a los errores discutidos. El analizador PVS-Studio tiene en cuenta
(std :: nothrow) y no da una advertencia si el objeto se crea de esta manera.
Puede parecer que tales errores son inofensivos. Bueno, piénselo, un cheque extra que nunca funciona. De todos modos, se lanzará una excepción que se procesará en alguna parte. Desafortunadamente, algunos desarrolladores colocan en la declaración verdadera de las acciones de
declaración if que liberan recursos, etc. Como este código no se está ejecutando, puede provocar pérdidas de memoria y otros errores.
Considere uno de estos casos que noté en el código de Android.
int parse_apk(const char *path, const char *target_package_name) { .... FileMap *dataMap = zip->createEntryFileMap(entry); if (dataMap == NULL) { ALOGW("%s: failed to create FileMap\n", __FUNCTION__); return -1; } char *buf = new char[uncompLen]; if (NULL == buf) { ALOGW("%s: failed to allocate %" PRIu32 " byte\n", __FUNCTION__, uncompLen); delete dataMap; return -1; } .... }
Advertencia de PVS-Studio: V668 CWE-570 No tiene sentido probar el puntero 'buf' contra nulo, ya que la memoria se asignó utilizando el operador 'nuevo'. La excepción se generará en caso de error de asignación de memoria. scan.cpp 213
Tenga en cuenta que si no es posible asignar un segundo bloque de memoria, el programador intenta liberar el primer bloque:
delete dataMap;
Este código nunca tendrá control. Este es un código muerto. Si se produce una excepción, se producirá una pérdida de memoria.
Escribir ese código es fundamentalmente incorrecto.
Se inventaron
punteros inteligentes para tales casos.
En total, el analizador PVS-Studio detectó
176 lugares en el código de Android donde se verifica el puntero después de crear objetos usando
nuevos . No entendí lo peligroso que es cada uno de estos lugares y, por supuesto, no saturaré el artículo con todas estas advertencias. Los
interesados pueden ver otras advertencias en el archivo
Android_V668.txt .
Desreferenciar un puntero nulo
Anular la referencia a un puntero nulo conduce a un comportamiento indefinido del programa, por lo que es útil encontrar y corregir dichos lugares. Dependiendo de la situación, el analizador PVS-Studio puede clasificar estos errores de acuerdo con la Enumeración de Debilidad Común de la siguiente manera:
- CWE-119 : Restricción incorrecta de operaciones dentro de los límites de un búfer de memoria
- CWE-476 : Desreferencia de puntero nulo
- CWE-628 : Llamada de función con argumentos incorrectamente especificados
- CWE-690 : Valor de retorno sin marcar a la referencia de puntero NULL
A menudo encuentro tales errores en el código responsable de manejar situaciones no estándar o incorrectas. Nadie prueba dicho código, y el error puede vivir en él durante mucho tiempo. Tal caso será considerado ahora.
bool parseEffect(....) { .... if (xmlProxyLib == nullptr) { ALOGE("effectProxy must contain a <%s>: %s", tag, dump(*xmlProxyLib)); return false; } .... }
Advertencia de PVS-Studio: V522 CWE-476 Puede tener lugar la desreferenciación del puntero nulo 'xmlProxyLib'. EffectsConfig.cpp 205
Si el puntero
xmlProxyLib es
nullptr , entonces el programador muestra un mensaje de depuración, para lo cual es necesario desreferenciar este puntero. Ups ...
Ahora considere una versión más interesante del error.
static void soinfo_unload_impl(soinfo* root) { .... soinfo* needed = find_library(si->get_primary_namespace(), library_name, RTLD_NOLOAD, nullptr, nullptr); if (needed != nullptr) {
Advertencia de PVS-Studio: V522 CWE-476 Puede producirse una desreferenciación del puntero nulo 'necesario'. linker.cpp 1847
Si se
necesita el puntero
! = Nullptr , se emite una advertencia, que es un comportamiento muy sospechoso del programa. Finalmente queda claro que el código contiene un error si mira a continuación y ve que cuando
sea necesario == nullptr , el puntero nulo se desreferenciará en la expresión necesaria-
> is_linked () .
Lo más probable es que los operadores! = Y == simplemente estén confundidos aquí. Si lo reemplaza, el código de función tiene sentido y el error desaparece.
La mayor parte de las advertencias sobre la posible desreferenciación de un puntero nulo se refiere a una situación de la forma:
T *p = (T *) malloc (N); *p = x;
Funciones como
malloc ,
strdup, etc. pueden devolver
NULL si no se puede asignar memoria. Por lo tanto, no puede desreferenciar los punteros que devuelven estas funciones sin verificar primero el puntero.
Hay muchos errores similares, por lo que solo daré dos fragmentos de código simples: el primero con
Malloc y el segundo con
strdup .
DownmixerBufferProvider::DownmixerBufferProvider(....) { .... effect_param_t * const param = (effect_param_t *) malloc(downmixParamSize); param->psize = sizeof(downmix_params_t); .... }
Advertencia de PVS-Studio: V522 CWE-690 Puede haber una desreferenciación de un puntero nulo potencial 'param'. Líneas de verificación: 245, 244. BufferProviders.cpp 245
static char* descriptorClassToDot(const char* str) { .... newStr = strdup(lastSlash); newStr[strlen(lastSlash)-1] = '\0'; .... }
Advertencia de PVS-Studio: V522 CWE-690 Puede haber una desreferenciación de un puntero nulo potencial 'newStr'. Líneas de verificación: 203, 202. DexDump.cpp 203
Alguien puede decir que estos son errores menores. Si no hay suficiente memoria, entonces el programa simplemente se bloqueará al desreferenciar el puntero nulo, y esto es normal. Como no hay memoria, no hay nada que intente de alguna manera manejar esta situación.
Tal persona está equivocada. Los punteros deben ser revisados! Examiné este tema en detalle en el artículo "
¿Por qué es importante verificar qué devolvió la función malloc? ". Le recomiendo que lo lea a todos los que no lo hayan leído.
En resumen, el peligro es que escribir en la memoria no necesariamente se produce cerca de la dirección cero. Es posible escribir datos en algún lugar muy alejado de una página de memoria que no esté protegida contra escritura y, por lo tanto, causar un error difícil de alcanzar, o en general este error puede usarse como una vulnerabilidad. Veamos a qué me refiero con el ejemplo de la función
check_size .
int check_size(radio_metadata_buffer_t **metadata_ptr, const uint32_t size_int) { .... metadata = realloc(metadata, new_size_int * sizeof(uint32_t)); memmove( (uint32_t *)metadata + new_size_int - (metadata->count + 1), (uint32_t *)metadata + metadata->size_int - (metadata->count + 1), (metadata->count + 1) * sizeof(uint32_t)); .... }
Advertencia de PVS-Studio: V769 CWE-119 El puntero '(uint32_t *) metadata' en la expresión '(uint32_t *) metadata + new_size_int' podría ser nullptr. En tal caso, el valor resultante no tendrá sentido y no debe usarse. Verifique las líneas: 91, 89. radio_metadata.c 91
No entendí la lógica de la función, pero esto no es necesario. Lo principal es que se crea un nuevo búfer y los datos se copian allí. Si la función
realloc devuelve
NULL , los datos se copiarán a la dirección ((uint32_t *) NULL + metadata-> size_int - (metadata-> count + 1)).
Si el
valor de metadata-> size_int es grande, entonces las consecuencias serán lamentables. Resulta que los datos se escriben en una memoria aleatoria.
Por cierto, hay otro tipo de anulación de referencia de puntero nulo, que el analizador PVS-Studio clasifica no como CWE-690, sino como CWE-628 (argumento no válido).
static void parse_tcp_ports(const char *portstring, uint16_t *ports) { char *buffer; char *cp; buffer = strdup(portstring); if ((cp = strchr(buffer, ':')) == NULL) .... }
Advertencia de PVS-Studio: V575 CWE-628 El puntero nulo potencial se pasa a la función 'strchr'. Inspecciona el primer argumento. Líneas de verificación: 47, 46. libxt_tcp.c 47
El hecho es que la desreferenciación del puntero ocurrirá cuando se
llame a la función
strchr . Por lo tanto, el analizador interpreta esta situación como pasar un valor incorrecto a la función.
Las
194 advertencias restantes de este tipo
figuran en el archivo
Android_V522_V575.txt .
Por cierto, las advertencias discutidas anteriormente sobre la comprobación del puntero después de llamar al
nuevo operador dan un picante especial a todos estos errores. Resulta que hay 195 llamadas a las
funciones malloc /
realloc /
strdup, y así sucesivamente, cuando el puntero no está marcado. Pero hay 176 lugares donde se marca el puntero después de llamar a
new . De acuerdo, un enfoque extraño!
Al final, nos queda considerar las advertencias V595 y V1004, que también están asociadas con el uso de punteros nulos.
V595 detecta situaciones en las que el puntero se desreferencia y luego se verifica. Ejemplo sintético:
p->foo(); if (!p) Error();
V1004 revela la situación inversa cuando el puntero se verificó primero y luego se olvidó de hacerlo. Ejemplo sintético:
if (p) p->foo(); p->doo();
Veamos algunos fragmentos de código de Android, donde hubo errores de este tipo.
Explicar especialmente sus características no son necesarias. PV_STATUS RC_UpdateBuffer(VideoEncData *video, Int currLayer, Int num_skip) { rateControl *rc = video->rc[currLayer]; MultiPass *pMP = video->pMP[currLayer]; if (video == NULL || rc == NULL || pMP == NULL) return PV_FAIL; .... }
Advertencia de PVS-Studio: V595 CWE-476 El puntero 'video' se utilizó antes de que se verificara contra nullptr. Verifique las líneas: 385, 388. rate_control.cpp 385 static void resampler_reset(struct resampler_itfe *resampler) { struct resampler *rsmp = (struct resampler *)resampler; rsmp->frames_in = 0; rsmp->frames_rq = 0; if (rsmp != NULL && rsmp->speex_resampler != NULL) { speex_resampler_reset_mem(rsmp->speex_resampler); } }
Advertencia de PVS-Studio: V595 CWE-476 El puntero 'rsmp' se utilizó antes de que se verificara contra nullptr. Verifique las líneas: 54, 57. resampler.c 54 void bta_gattc_disc_cmpl(tBTA_GATTC_CLCB* p_clcb, UNUSED_ATTR tBTA_GATTC_DATA* p_data) { .... if (p_clcb->status != GATT_SUCCESS) { if (p_clcb->p_srcb) { std::vector<tBTA_GATTC_SERVICE>().swap( p_clcb->p_srcb->srvc_cache); } bta_gattc_cache_reset(p_clcb->p_srcb->server_bda); } .... }
Advertencia de PVS-Studio: V1004 CWE-476 El puntero 'p_clcb-> p_srcb' se usó de manera insegura después de que se verificó contra nullptr. Verifique las líneas: 695, 701. bta_gattc_act.cc 701No es interesante considerar otras advertencias de este tipo. Entre ellos hay errores y advertencias falsas que surgen debido a un código mal o difícilmente escrito.Escribí una docena de advertencias útiles:- V1004 CWE-476 The 'ain' pointer was used unsafely after it was verified against nullptr. Check lines: 101, 105. rsCpuIntrinsicBLAS.cpp 105
- V595 CWE-476 The 'outError' pointer was utilized before it was verified against nullptr. Check lines: 437, 450. Command.cpp 437
- V595 CWE-476 The 'out_last_reference' pointer was utilized before it was verified against nullptr. Check lines: 432, 436. AssetManager2.cpp 432
- V595 CWE-476 The 'set' pointer was utilized before it was verified against nullptr. Check lines: 4524, 4529. ResourceTypes.cpp 4524
- V595 CWE-476 The 'reply' pointer was utilized before it was verified against nullptr. Check lines: 126, 133. Binder.cpp 126
- V595 CWE-476 The 'video' pointer was utilized before it was verified against nullptr. Check lines: 532, 540. rate_control.cpp 532
- V595 CWE-476 The 'video' pointer was utilized before it was verified against nullptr. Check lines: 702, 711. rate_control.cpp 702
- V595 CWE-476 The 'pInfo' pointer was utilized before it was verified against nullptr. Check lines: 251, 254. ResolveInfo.cpp 251
- V595 CWE-476 The 'address' pointer was utilized before it was verified against nullptr. Check lines: 53, 55. DeviceHalHidl.cpp 53
- V595 CWE-476 The 'halAddress' pointer was utilized before it was verified against nullptr. Check lines: 55, 82. DeviceHalHidl.cpp 55
Y luego me aburrí y filtré advertencias de este tipo. Por lo tanto, ni siquiera sé cuántos errores reales ha detectado el analizador. Estas advertencias están esperando a su héroe, que las estudiará cuidadosamente y hará cambios en el código.Solo quiero llamar la atención de mis nuevos lectores sobre errores de este tipo: NJ_EXTERN NJ_INT16 njx_search_word(NJ_CLASS *iwnn, ....) { .... NJ_PREVIOUS_SELECTION_INFO *prev_info = &(iwnn->previous_selection); if (iwnn == NULL) { return NJ_SET_ERR_VAL(NJ_FUNC_NJ_SEARCH_WORD, NJ_ERR_PARAM_ENV_NULL); } .... }
Advertencia de PVS-Studio: V595 CWE-476 El puntero 'iwnn' se utilizó antes de que se verificara contra nullptr. Verifique las líneas: 686, 689. ndapi.c 686Algunas personas piensan que no hay ningún error aquí, porque "no hay una desreferencia real del puntero". La dirección de una variable inexistente simplemente se calcula. Además, si el puntero iwnn es cero, la función se cerrará. Por lo tanto, no sucedió nada malo que previamente calculáramos incorrectamente la dirección de un miembro de la clase.No, no puedes hablar así. Este código conduce a un comportamiento indefinido y, por lo tanto, no se puede escribir así. El comportamiento indefinido puede manifestarse, por ejemplo, de la siguiente manera:- El compilador ve el puntero desreferenciado: iwnn-> previous_selection
- No puede desreferenciar un puntero nulo, porque este es un comportamiento indefinido
- El compilador concluye que el puntero iwnn siempre es distinto de cero
- El compilador elimina la comprobación adicional: if (iwnn == NULL)
- Eso es todo, ahora cuando se ejecuta el programa, la comprobación del puntero nulo no se realiza y el trabajo comienza con el puntero incorrecto para el miembro de la clase
Este tema se describe con más detalle en mi artículo " Desreferenciar un puntero nulo conduce a un comportamiento indefinido ".Los datos privados no se borran en la memoria
Considere un tipo grave de vulnerabilidad potencial que se clasifica según la Enumeración de debilidad común como CWE-14 : Eliminación del código del compilador para borrar los buffers.En resumen, la conclusión es esta: el compilador tiene el derecho de eliminar la llamada a la función memset si después de eso el búfer ya no se usa.Cuando escribo sobre este tipo de vulnerabilidad, los comentarios necesariamente aparecen que esto es solo una falla en el compilador que debe corregirse. No, esto no es un problema técnico. Y antes de objetar, lea los siguientes materiales:En general, todo es serio. ¿Hay algún error de este tipo en Android? Por supuesto que lo hay.
Generalmente son muchos donde hay: prueba :).Volvamos al código de Android y veamos el principio y el final de la función FwdLockGlue_InitializeRoundKeys , ya que el medio no es interesante para nosotros. static void FwdLockGlue_InitializeRoundKeys() { unsigned char keyEncryptionKey[KEY_SIZE]; .... memset(keyEncryptionKey, 0, KEY_SIZE);
Advertencia de PVS-Studio: V597 CWE-14 El compilador podría eliminar la llamada a la función 'memset', que se utiliza para vaciar el búfer 'keyEncryptionKey'. La función memset_s () debe usarse para borrar los datos privados. FwdLockGlue.c 102Matriz keyEncryptionKey se crea en la pila y almacena información privada. Al final de la función, quieren llenar esta matriz con ceros para que no llegue accidentalmente a donde no debería. Cómo la información puede llegar a donde no debería, el artículo " Sobrescribir memoria, ¿por qué? " Lo dirá .Para llenar una matriz con información privada con ceros, se utiliza la función memset . El comentario "Cero datos clave" confirma que entendemos todo correctamente.El problema es que con una probabilidad muy alta, el compilador eliminará la llamada a la función memset al crear la versión de lanzamiento . Dado que el búfer después de la llamada memset no se usa, la llamada de la función memset en sí es superflua desde el punto de vista del compilador.Más de 10 advertencias yo escribimos un archivo Android_V597.txt .Hubo otro error en el que la memoria no se sobrescribe, aunque en este caso la función memset no tiene nada que ver con eso. void SHA1Transform(uint32_t state[5], const uint8_t buffer[64]) { uint32_t a, b, c, d, e; .... a = b = c = d = e = 0; }
Advertencia de PVS-Studio: V1001 CWE-563 La variable 'a' se asigna pero no se usa hasta el final de la función. sha1.c 213PVS-Studio detectó una anomalía relacionada con el hecho de que después de asignar valores a las variables, ya no se usan. El analizador clasificó este defecto como CWE-563 : Asignación a variable sin uso. Y, formalmente, tiene razón, aunque, de hecho, aquí estamos nuevamente tratando con CWE-14. El compilador desechará estas tareas, ya que desde el punto de vista de C y C ++ son superfluas. Como resultado, la pila retendrá los valores antiguos de las variables a , b , c , d y e , que almacenan datos privados.Comportamiento no especificado / definido por la implementación
Si bien no estás cansado, veamos un caso complejo que requerirá una descripción completa de mi parte. typedef int32_t GGLfixed; GGLfixed gglFastDivx(GGLfixed n, GGLfixed d) { if ((d>>24) && ((d>>24)+1)) { n >>= 8; d >>= 8; } return gglMulx(n, gglRecip(d)); }
Advertencia de PVS-Studio: V793 Es extraño que el resultado de la declaración '(d >> 24) + 1' sea parte de la condición. Quizás, esta declaración debería haber sido comparada con otra cosa. fixed.cpp 75El programador quería comprobar que los 8 bits de orden superior de la variable d contienen unidades, pero no todos los bits a la vez. En otras palabras, el programador quería verificar que cualquier valor que no sea 0x00 y 0xFF esté en el byte alto.Se acercó a la solución de este problema innecesariamente creativamente. Para empezar, verificó que los bits más significativos son distintos de cero escribiendo una expresión (d >> 24). Hay afirmaciones sobre esta expresión, pero es más interesante analizar el lado derecho de la expresión: ((d >> 24) +1). El programador desplaza los ocho bits altos al byte bajo. Al mismo tiempo, calcula que el bit de signo más significativo está duplicado en todos los demás bits. Es decir
si la variable d es 0b11111111'00000000'00000000'00000000, entonces después del cambio obtenemos el valor 0b11111111'11111111'11111111'11111111. Agregando 1 al valor 0xFFFFFFFF de tipo int , el programador planea obtener 0. Es decir: -1 + 1 = 0. Por lo tanto, con la expresión ((d >> 24) +1), comprueba que no todos los ocho bits altos son iguales a 1. Entiendo que esto es bastante difícil, así que tómate tu tiempo e intenta averiguar cómo y qué funciona :).Ahora veamos qué está mal con este código.Al cambiar, el bit de signo más significativo no necesariamente está "manchado". Esto es lo que dice el estándar al respecto: “El valor de E1 >> E2 es E1 posiciones de bit E2 desplazadas a la derecha. Si E1 tiene un tipo sin signo o si E1 tiene un tipo con signo y un valor no negativo, el valor del resultado es la parte integral del cociente de E1 / 2 ^ E2. Si E1 tiene un tipo con signo y un valor negativo, el valor resultante está definido por la implementación ".La última oferta es importante para nosotros. Entonces, conocimos el comportamiento definido por la implementación. El funcionamiento de este código depende de la arquitectura del microprocesador y la implementación del compilador. Después del cambio, los ceros pueden aparecer en los bits más significativos, y luego la expresión ((d >> 24) +1) siempre será diferente de 0, es decir. Siempre será un verdadero valor.De ahí la conclusión: no es necesario ser sabio. El código será más confiable y comprensible si escribe, por ejemplo, así: GGLfixed gglFastDivx(GGLfixed n, GGLfixed d) { uint32_t hibits = static_cast<uint32_t>(d) >> 24; if (hibits != 0x00 && hibits != 0xFF) { n >>= 8; d >>= 8; } return gglMulx(n, gglRecip(d)); }
Probablemente, sugerí que no es la opción ideal, pero en este código no hay un comportamiento definido por la implementación, y será más fácil para el lector comprender lo que se está comprobando.Te mereces una taza de café o té. Distraer y continuar: estamos esperando un caso interesante de comportamiento no especificado.En la entrevista, como una de las primeras preguntas al solicitante, le pregunto lo siguiente: "¿Qué imprimirá la función printf y por qué?" int i = 5; printf("%d,%d", i++, i++)
La respuesta correcta: este es un comportamiento no especificado. El procedimiento para calcular los argumentos reales cuando se llama a la función no está definido. Ocasionalmente, incluso demuestro que este código, compilado con la ayuda de Visual C ++, muestra "6.5" en la pantalla, lo que confunde a los recién llegados que son débiles en conocimiento y espíritu a un punto muerto :).Esto puede parecer un problema descabellado. Pero no, este código se puede encontrar en aplicaciones serias, por ejemplo, en el código de Android. bool ComposerClient::CommandReader::parseSetLayerCursorPosition( uint16_t length) { if (length != CommandWriterBase::kSetLayerCursorPositionLength) { return false; } auto err = mHal.setLayerCursorPosition(mDisplay, mLayer, readSigned(), readSigned()); if (err != Error::NONE) { mWriter.setError(getCommandLoc(), err); } return true; }
Advertencia de PVS-Studio: V681 CWE-758 El estándar de lenguaje no define un orden en el que se invocarán las funciones 'readSigned' durante la evaluación de los argumentos. ComposerClient.cpp 836Estamos interesados en esta línea de código: mHal.setLayerCursorPosition(...., readSigned(), readSigned());
Al llamar a la función readSigned , se leen dos valores. Pero es imposible predecir en qué orden se leerán los valores. Este es un caso clásico de comportamiento no especificado.Beneficios de usar un analizador de código estático
Este artículo completo populariza el análisis de código estático en general y nuestra herramienta PVS-Studio en particular. Sin embargo, algunos errores son perfectos para demostrar las posibilidades del análisis estático. Son difíciles de identificar mediante revisiones de código, y solo un programa no cansado los nota fácilmente. Considere un par de tales casos. const std::map<std::string, int32_t> kBootReasonMap = { .... {"watchdog_sdi_apps_reset", 106}, {"smpl", 107}, {"oem_modem_failed_to_powerup", 108}, {"reboot_normal", 109}, {"oem_lpass_cfg", 110},
Advertencias de PVS-Studio:- V766 CWE-462 Ya se ha agregado un elemento con la misma clave '"oem_lpass_cfg"'. bootstat.cpp 264
- V766 CWE-462 Ya se ha agregado un elemento con la misma clave '"oem_xpu_ns_error"'. bootstat.cpp 265
Se insertan diferentes valores con las mismas claves en el contenedor asociativo ordenado ( std :: map ). En términos de enumeración de debilidad común, este es el CWE-462 : clave duplicada en la lista asociativa.El texto del programa se acorta y los errores están marcados con comentarios, por lo que el error parece obvio, pero cuando solo lee este código con los ojos, es muy difícil encontrar tales errores.Considere otra pieza de código que es muy difícil de leer, ya que es del mismo tipo y poco interesante. MtpResponseCode MyMtpDatabase::getDevicePropertyValue(....) { .... switch (type) { case MTP_TYPE_INT8: packet.putInt8(longValue); break; case MTP_TYPE_UINT8: packet.putUInt8(longValue); break; case MTP_TYPE_INT16: packet.putInt16(longValue); break; case MTP_TYPE_UINT16: packet.putUInt16(longValue); break; case MTP_TYPE_INT32: packet.putInt32(longValue); break; case MTP_TYPE_UINT32: packet.putUInt32(longValue); break; case MTP_TYPE_INT64: packet.putInt64(longValue); break; case MTP_TYPE_UINT64: packet.putUInt64(longValue); break; case MTP_TYPE_INT128: packet.putInt128(longValue); break; case MTP_TYPE_UINT128: packet.putInt128(longValue);
Advertencia de PVS-Studio: V525 CWE-682 El código contiene la colección de bloques similares. Verifique los elementos 'putInt8', 'putUInt8', 'putInt16', 'putUInt16', 'putInt32', 'putUInt32', 'putInt64', 'putUInt64', 'putInt128', 'putInt128' en las líneas 620, 623, 626, 629 , 632, 635, 638, 641, 644, 647. 620 android_mtp_MtpDatabase.cppSi MTP_TYPE_UINT128 tuvo que ser causada por una función putUInt128 , no putInt128 .Y el último en esta sección es un gran ejemplo de Copiar-Pegar sin éxito. static void btif_rc_upstreams_evt(....) { .... case AVRC_PDU_REQUEST_CONTINUATION_RSP: { BTIF_TRACE_EVENT( "%s() REQUEST CONTINUATION: target_pdu: 0x%02d", __func__, pavrc_cmd->continu.target_pdu); tAVRC_RESPONSE avrc_rsp; if (p_dev->rc_connected == TRUE) { memset(&(avrc_rsp.continu), 0, sizeof(tAVRC_NEXT_RSP)); avrc_rsp.continu.opcode = opcode_from_pdu(AVRC_PDU_REQUEST_CONTINUATION_RSP); avrc_rsp.continu.pdu = AVRC_PDU_REQUEST_CONTINUATION_RSP; avrc_rsp.continu.status = AVRC_STS_NO_ERROR; avrc_rsp.continu.target_pdu = pavrc_cmd->continu.target_pdu; send_metamsg_rsp(p_dev, -1, label, ctype, &avrc_rsp); } } break; case AVRC_PDU_ABORT_CONTINUATION_RSP: { BTIF_TRACE_EVENT( "%s() ABORT CONTINUATION: target_pdu: 0x%02d", __func__, pavrc_cmd->abort.target_pdu); tAVRC_RESPONSE avrc_rsp; if (p_dev->rc_connected == TRUE) { memset(&(avrc_rsp.abort), 0, sizeof(tAVRC_NEXT_RSP)); avrc_rsp.abort.opcode = opcode_from_pdu(AVRC_PDU_ABORT_CONTINUATION_RSP); avrc_rsp.abort.pdu = AVRC_PDU_ABORT_CONTINUATION_RSP; avrc_rsp.abort.status = AVRC_STS_NO_ERROR; avrc_rsp.abort.target_pdu = pavrc_cmd->continu.target_pdu; send_metamsg_rsp(p_dev, -1, label, ctype, &avrc_rsp); } } break; .... }
Antes de leer la advertencia del analizador y el texto adicional, sugiero buscar el error usted mismo.Para no leer accidentalmente la respuesta de inmediato, aquí hay una imagen para que desvíe la atención. Si está interesado en lo que significa un huevo con la inscripción Java, aquí está .Entonces, espero que hayan disfrutado la búsqueda de errores tipográficos. Ahora es el momento de avisar al analizador: V778 CWE-682 Se encontraron dos fragmentos de código similares. Quizás, este es un error tipográfico y se debe usar la variable 'abortar' en lugar de 'continuar'. btif_rc.cc 1554Aparentemente, el código fue escrito usando el método Copiar-Pegar, y una persona, como siempre, no pudo estar atenta en el proceso de edición del fragmento de código copiado. Como resultado, al final, no reemplazó " continu " con " abortar ".Es decir
en el segundo bloque debe escribirse: avrc_rsp.abort.target_pdu = pavrc_cmd->abort.target_pdu;
Esta situación cae completamente bajo la definición de " efecto de última línea ", ya que al final se cometió un error al reemplazar los nombres.Facepalm
Un error muy divertido está relacionado con la conversión entre formatos de datos little-endian y big-endian (ver orden de bytes ). inline uint32_t bswap32(uint32_t pData) { return (((pData & 0xFF000000) >> 24) | ((pData & 0x00FF0000) >> 8) | ((pData & 0x0000FF00) << 8) | ((pData & 0x000000FF) << 24)); } bool ELFAttribute::merge(....) { .... uint32_t subsection_length = *reinterpret_cast<const uint32_t*>(subsection_data); if (llvm::sys::IsLittleEndianHost != m_Config.targets().isLittleEndian()) bswap32(subsection_length); .... }
Advertencia de PVS-Studio: V530 CWE-252 Se requiere utilizar el valor de retorno de la función 'bswap32'. ELFAttribute.cpp 84No hay quejas sobre la función bswap32 . Pero se usa incorrectamente: bswap32(subsection_length);
El autor sugirió que la variable se pasa a la función por referencia y se cambia allí. Sin embargo, debe usar el valor devuelto por la función. Como resultado, no se produce conversión de datos.El analizador identificó este error como CWE-252 : Valor de retorno no verificado. Pero, de hecho, es más apropiado llamar a CWE-198 : Uso de orden de bytes incorrecto aquí. Desafortunadamente, el analizador no puede entender cuál es el error desde un punto de vista de alto nivel. Sin embargo, esto no le impide identificar este defecto grave en el código.El código correcto es: subsection_length = bswap32(subsection_length);
Hay tres lugares más en el código de Android con el mismo error:- V530 CWE-252 Se requiere utilizar el valor de retorno de la función 'bswap32'. ELFAttribute.cpp 218
- V530 CWE-252 Se requiere utilizar el valor de retorno de la función 'bswap32'. ELFAttribute.cpp 346
- V530 CWE-252 Se requiere utilizar el valor de retorno de la función 'bswap32'. ELFAttribute.cpp 352
Para evitar tales errores, se recomienda usar el atributo [[nodiscard]] . Este atributo se usa para indicar que el valor de retorno de una función debe usarse cuando se invoca. Por lo tanto, si escribes así: [[nodiscard]] inline uint32_t bswap32(uint32_t pData) { ... }
entonces el error se detectaría incluso en la etapa de compilación del archivo. Puede obtener más información sobre algunos nuevos atributos útiles del artículo de mi colega " C ++ 17 ".Código inalcanzable
En la programación y la teoría del compilador, el código inalcanzable es la parte del código del programa que no puede ejecutarse bajo ninguna circunstancia, ya que no es accesible en el gráfico de flujo de control.Desde el punto de vista de la Enumeración de Debilidad Común, es CWE-561 : Código Muerto. virtual sp<IEffect> createEffect(....) { .... if (pDesc == NULL) { return effect; if (status != NULL) { *status = BAD_VALUE; } } .... }
Advertencia de PVS-Studio: V779 CWE-561 Código inalcanzable detectado. Es posible que haya un error presente. IAudioFlinger.cpp 733La declaración de devolución debe ubicarse explícitamente a continuación.Otros errores de este tipo:- V779 CWE-561 Código inalcanzable detectado. Es posible que haya un error presente. bta_hf_client_main.cc 612
- V779 CWE-561 Código inalcanzable detectado. Es posible que haya un error presente. android_media_ImageReader.cpp 468
- V779 CWE-561 Código inalcanzable detectado. Es posible que haya un error presente. AMPEG4AudioAssembler.cpp 187
romper
La interrupción olvidada dentro del interruptor es un error clásico de los programadores C y C ++. Para combatirlo, una anotación útil apareció en C ++ 17 como [[fallthrough]] . Puede leer más sobre este error y [[fallthrough]] en mi artículo " break and fallthrough ".Pero mientras el mundo está lleno de código antiguo donde [[fallthrough]] no se usa, PVS-Studio es útil para usted. Considere algunos errores encontrados en Android. De acuerdo con la enumeración de debilidad común, estos errores se clasifican como CWE-484 : Declaración de interrupción omitida en el interruptor. bool A2dpCodecConfigLdac::setCodecConfig(....) { .... case BTAV_A2DP_CODEC_SAMPLE_RATE_192000: if (sampleRate & A2DP_LDAC_SAMPLING_FREQ_192000) { result_config_cie.sampleRate = A2DP_LDAC_SAMPLING_FREQ_192000; codec_capability_.sample_rate = codec_user_config_.sample_rate; codec_config_.sample_rate = codec_user_config_.sample_rate; } case BTAV_A2DP_CODEC_SAMPLE_RATE_16000: case BTAV_A2DP_CODEC_SAMPLE_RATE_24000: case BTAV_A2DP_CODEC_SAMPLE_RATE_NONE: codec_capability_.sample_rate = BTAV_A2DP_CODEC_SAMPLE_RATE_NONE; codec_config_.sample_rate = BTAV_A2DP_CODEC_SAMPLE_RATE_NONE; break; .... }
Advertencia de PVS-Studio: V796 CWE-484 Es posible que falte la declaración 'break' en la declaración del interruptor. a2dp_vendor_ldac.cc 912Creo que el error no necesita explicación. Solo noto que a menudo se detecta una anomalía en el código de más de una manera. Por ejemplo, este error también es detectado por las advertencias V519:- V519 CWE-563 La variable 'codec_capability_.sample_rate' recibe valores asignados dos veces sucesivamente. Quizás esto sea un error. Líneas de verificación: 910, 916. a2dp_vendor_ldac.cc 916
- V519 CWE-563 La variable 'codec_config_.sample_rate' tiene valores asignados dos veces sucesivamente. Quizás esto sea un error. Líneas de verificación: 911, 917. a2dp_vendor_ldac.cc 917
Y un par de errores más: Return<void> EffectsFactory::getAllDescriptors(....) { .... switch (status) { case -ENOSYS: {
Advertencia de PVS-Studio: V796 CWE-484 Es posible que falte la declaración 'break' en la declaración del interruptor. EffectsFactory.cpp 118 int Reverb_getParameter(....) { .... case REVERB_PARAM_REFLECTIONS_LEVEL: *(uint16_t *)pValue = 0; case REVERB_PARAM_REFLECTIONS_DELAY: *(uint32_t *)pValue = 0; case REVERB_PARAM_REVERB_DELAY: *(uint32_t *)pValue = 0; break; .... }
Advertencia de PVS-Studio: V796 CWE-484 Es posible que falte la declaración 'break' en la declaración del interruptor. EffectReverb.cpp 1847 static SLresult IAndroidConfiguration_GetConfiguration(....) { .... switch (IObjectToObjectID((thiz)->mThis)) { case SL_OBJECTID_AUDIORECORDER: result = android_audioRecorder_getConfig( (CAudioRecorder *) thiz->mThis, configKey, pValueSize, pConfigValue); break; case SL_OBJECTID_AUDIOPLAYER: result = android_audioPlayer_getConfig( (CAudioPlayer *) thiz->mThis, configKey, pValueSize, pConfigValue); default: result = SL_RESULT_FEATURE_UNSUPPORTED; break; } .... }
Advertencia de PVS-Studio: V796 CWE-484 Es posible que falte la declaración 'break' en la declaración del interruptor. IAndroidConfiguration.cpp 90Gestión de memoria incorrecta
Aquí he compilado errores relacionados con la gestión incorrecta de la memoria. Dichas advertencias, de acuerdo con la enumeración de debilidad común, se clasifican como:- CWE-401 : Liberación incorrecta de memoria antes de eliminar la última referencia ('Fuga de memoria')
- CWE-562 : Devolución de dirección variable de pila
- CWE-762 : Rutinas de administración de memoria no coincidentes
Comencemos con funciones que devuelven una referencia a una variable ya destruida. TransformIterator& operator++(int) { TransformIterator tmp(*this); ++*this; return tmp; } TransformIterator& operator--(int) { TransformIterator tmp(*this); --*this; return tmp; }
Advertencias de PVS-Studio:- La función V558 CWE-562 devuelve la referencia al objeto local temporal: tmp. transform_iterator.h 77
- La función V558 CWE-562 devuelve la referencia al objeto local temporal: tmp. transform_iterator.h 92
Cuando las funciones finalizan su ejecución, la variable tmp se destruye, ya que se creó en la pila. En consecuencia, las funciones devuelven una referencia a un objeto ya destruido (inexistente).La solución correcta sería devolver un valor de: TransformIterator operator++(int) { TransformIterator tmp(*this); ++*this; return tmp; } TransformIterator operator--(int) { TransformIterator tmp(*this); --*this; return tmp; }
Considere un código aún más triste que merece mucha atención. int register_socket_transport( int s, const char* serial, int port, int local) { atransport* t = new atransport(); if (!serial) { char buf[32]; snprintf(buf, sizeof(buf), "T-%p", t); serial = buf; } .... }
Advertencia de PVS-Studio: V507 CWE-562 El puntero a la matriz local 'buf' se almacena fuera del alcance de esta matriz. Tal puntero se volverá inválido. transport.cpp 1030Este es un código peligroso. Si el valor real del argumento en serie es NULL, se debe usar un búfer temporal en la pila. Cuando el cuerpo de la instrucción if finaliza, la matriz buf deja de existir. El lugar donde se creó el búfer se puede usar para almacenar otras variables creadas en la pila. Comenzará un desastre infernal en los datos, y las consecuencias de tal error están mal predichas.Los siguientes errores están relacionados con métodos incompatibles para crear y destruir objetos. void SensorService::SensorEventConnection::reAllocateCacheLocked(....) { sensors_event_t *eventCache_new; const int new_cache_size = computeMaxCacheSizeLocked(); eventCache_new = new sensors_event_t[new_cache_size]; .... delete mEventCache; mEventCache = eventCache_new; mCacheSize += count; mMaxCacheSize = new_cache_size; }
Advertencia de PVS-Studio: V611 CWE-762 La memoria se asignó usando el operador 'nuevo T []' pero se liberó usando el operador 'borrar'. Considere inspeccionar este código. Probablemente sea mejor usar 'delete [] mEventCache;'. Verifique las líneas: 391, 384. SensorEventConnection.cpp 391 Aquítodo es simple. El búfer al que apunta un miembro de la clase mEventCache se asigna utilizando el nuevo operador [] . Y libere esta memoria utilizando el operador de eliminación . Esto es incorrecto y conduce a un comportamiento indefinido del programa.Error similar aaudio_result_t AAudioServiceEndpointCapture::open(....) { .... delete mDistributionBuffer; int distributionBufferSizeBytes = getStreamInternal()->getFramesPerBurst() * getStreamInternal()->getBytesPerFrame(); mDistributionBuffer = new uint8_t[distributionBufferSizeBytes]; .... }
Advertencia de PVS-Studio: V611 CWE-762 La memoria se asignó usando el operador 'nuevo T []' pero se liberó usando el operador 'borrar'. Considere inspeccionar este código. Probablemente sea mejor usar 'delete [] mDistributionBuffer;'. AAudioServiceEndpointCapture.cpp 50Creo que el error no requiere explicación.El siguiente caso es un poco más interesante, pero la esencia del error es exactamente la misma. struct HeifFrameInfo { .... void set(....) { .... mIccData.reset(new uint8_t[iccSize]); .... } .... std::unique_ptr<uint8_t> mIccData; };
V554 CWE-762 Uso incorrecto de unique_ptr. La memoria asignada con 'nuevo []' se limpiará con 'eliminar'.
HeifDecoderAPI.h 62De manera predeterminada, la clase de puntero inteligente std :: unique_ptr llama al operador de eliminación para destruir un objeto . Sin embargo, en la función set , la memoria se asigna usando el nuevo operador [] .La opción correcta:
std::unique_ptr<uint8_t[]> mIccData;
Otros errores:
- V554 CWE-762 Uso incorrecto de unique_ptr. La memoria asignada con 'nuevo []' se limpiará con 'eliminar'. atrace.cpp 949
- V554 CWE-762 Uso incorrecto de unique_ptr. La memoria asignada con 'nuevo []' se limpiará con 'eliminar'. atrace.cpp 950
- V554 CWE-762 Uso incorrecto de unique_ptr. La memoria asignada con 'nuevo []' se limpiará con 'eliminar'. HeifDecoderImpl.cpp 102
- V554 CWE-762 Uso incorrecto de unique_ptr. La memoria asignada con 'nuevo []' se limpiará con 'eliminar'. HeifDecoderImpl.cpp 166
- V554 CWE-762 Uso incorrecto de unique_ptr. La memoria asignada con 'nuevo []' se limpiará con 'eliminar'. ColorSpace.cpp 360
Los errores de pérdida de memoria completarán esta sección. Una sorpresa desagradable es que hubo más de 20 errores de este tipo. Me parece que estos pueden ser defectos muy dolorosos, que conducen a una disminución gradual de la memoria libre durante el funcionamiento continuo prolongado del sistema operativo. Asset* Asset::createFromUncompressedMap(FileMap* dataMap, AccessMode mode) { _FileAsset* pAsset; status_t result; pAsset = new _FileAsset; result = pAsset->openChunk(dataMap); if (result != NO_ERROR) return NULL; pAsset->mAccessMode = mode; return pAsset; }
Advertencia de PVS-Studio: V773 CWE-401 La función se cerró sin soltar el puntero 'pAsset'. Una pérdida de memoria es posible.
Asset.cpp 296Si no fue posible abrir un determinado fragmento, la función se cierra sin destruir el objeto, cuyo puntero se almacena en la variable pAsset . Como resultado, se producirá una pérdida de memoria.Otros errores son similares, por lo que no veo ninguna razón para considerarlos en el artículo. Los interesados pueden ver otras advertencias en el archivo: Android_V773.txt .Ir al extranjero una gran variedad
Hay una gran cantidad de patrones erróneos que conducen al desbordamiento de la matriz. En el caso de Android, detecté solo un patrón de error del siguiente tipo: if (i < 0 || i > MAX) return; A[i] = x;
En C y C ++, las celdas de la matriz están numeradas de 0, por lo que el índice máximo de un elemento en la matriz debe ser uno menor que el tamaño de la matriz en sí. La verificación correcta debería ser así: if (i < 0 || i >= MAX) return; A[i] = x;
El desbordamiento de una matriz, de acuerdo con la Enumeración de Debilidad Común, se clasifica como CWE-119 : Restricción inadecuada de operaciones dentro de los límites de un buffer de memoria.Eche un vistazo a cómo se ven estos errores en el código de Android. static btif_hf_cb_t btif_hf_cb[BTA_AG_MAX_NUM_CLIENTS]; static bool IsSlcConnected(RawAddress* bd_addr) { if (!bd_addr) { LOG(WARNING) << __func__ << ": bd_addr is null"; return false; } int idx = btif_hf_idx_by_bdaddr(bd_addr); if (idx < 0 || idx > BTA_AG_MAX_NUM_CLIENTS) { LOG(WARNING) << __func__ << ": invalid index " << idx << " for " << *bd_addr; return false; } return btif_hf_cb[idx].state == BTHF_CONNECTION_STATE_SLC_CONNECTED; }
Advertencia de PVS-Studio: V557 CWE-119 Array overrun es posible. El valor del índice 'idx' podría alcanzar 6. btif_hf.cc 277Opción de verificación correcta: if (idx < 0 || idx >= BTA_AG_MAX_NUM_CLIENTS) {
Exactamente los mismos errores se encontraron dos cosas más:- V557 CWE-119 Arreglo de arrastre es posible. El valor del índice 'idx' podría alcanzar 6. btif_hf.cc 869
- V557 CWE-119 Arreglo de arrastre es posible. El valor del índice 'index' podría alcanzar 6. btif_rc.cc 374
Ciclos rotos
Hay muchas formas de escribir un ciclo de mal funcionamiento. En el código de Android, encontré errores que, de acuerdo con la enumeración de debilidad común, se pueden clasificar como:- CWE-20 : Validación de entrada incorrecta
- CWE-670 : Implementación de flujo de control siempre incorrecta
- CWE-691 : Gestión de flujo de control insuficiente
- CWE-834 : iteración excesiva
Al mismo tiempo, por supuesto, hay otras formas de "dispararle a su propia pierna" al escribir ciclos, pero esta vez no me conocieron. int main(int argc, char **argv) { .... char c; printf("%s is already in *.base_fs format, just ..... ", ....); rewind(blk_alloc_file); while ((c = fgetc(blk_alloc_file)) != EOF) { fputc(c, base_fs_file); } .... }
Advertencia de PVS-Studio: V739 CWE-20 EOF no debe compararse con un valor del tipo 'char'. El '(c = fgetc (blk_alloc_file))' debería ser del tipo 'int'. blk_alloc_to_base_fs.c 61El analizador detectó que la constante EOF se compara con una variable de tipo char . Veamos por qué este código es incorrecto.La función fgetc devuelve un valor int , a saber: puede devolver un número de 0 a 255 o EOF (-1). El valor de lectura se coloca en una variable de tipo char . Debido a esto, un carácter con un valor de 0xFF (255) se convierte en -1 y se interpreta exactamente de la misma manera que el final de un archivo (EOF).Debido a tales errores, los usuarios que utilizan códigos ASCII extendidos a veces se encuentran con una situación en la que los programas procesan incorrectamente uno de los caracteres de su alfabeto. Por ejemplo, la última letra del alfabeto ruso codificada en Windows-1251 solo tiene el código 0xFF y algunos programas la perciben como el final del archivo.En resumen, podemos decir que la condición para detener el ciclo está escrita incorrectamente. Para solucionar la situación, la variable c debe ser de tipo int.Continuamos y consideramos errores más familiares cuando utilizamos la declaración for . status_t AudioPolicyManager::registerPolicyMixes(....) { .... for (size_t i = 0; i < mixes.size(); i++) { .... for (size_t j = 0; i < mHwModules.size(); j++) {
Advertencia de PVS-Studio: V534 CWE-691 Es probable que se esté comparando una variable incorrecta dentro del operador 'for'. Considere revisar 'i'. AudioPolicyManager.cpp 2489Debido a un error tipográfico en un bucle anidado, la condición usa la variable i , aunque debe usar la variable j . Como resultado, la variable j se incrementa incontrolablemente, lo que con el tiempo llevará a que la matriz mHwModules se salga de los límites . Lo que sucederá después es imposible de predecir, ya que habrá un comportamiento indefinido del programa.Por cierto, este fragmento con un error se copió completamente a otra función. Por lo tanto, el analizador encontró exactamente el mismo error aquí: AudioPolicyManager.cpp 2586.Hay 3 fragmentos de código más que son muy sospechosos para mí. Sin embargo, no presumo decir que este código es definitivamente erróneo, ya que allí hay una lógica compleja. En cualquier caso, debo prestar atención a este código para que el autor lo revise.El primer fragmento: void ce_t3t_handle_check_cmd(....) { .... for (i = 0; i < p_cb->cur_cmd.num_blocks; i++) { .... for (i = 0; i < T3T_MSG_NDEF_ATTR_INFO_SIZE; i++) { checksum += p_temp[i]; } .... } .... }
Advertencia de PVS-Studio: V535 CWE-691 La variable 'i' se está utilizando para este bucle y para el bucle externo. Verifique las líneas: 398, 452. ce_t3t.cc 452Tenga en cuenta que la variable i se usa tanto para los ciclos externos como internos.Dos disparadores más similares del analizador:- V535 CWE-691 La variable 'xx' se está utilizando para este bucle y para el bucle externo. Verifique las líneas: 801, 807. sdp_db.cc 807
- V535 CWE-691 La variable 'xx' se está utilizando para este bucle y para el bucle externo. Líneas de verificación: 424, 438. nfa_hci_act.cc 438
¿Aún no estás cansado? Sugiero pausar y descargar PVS-Studio para intentar verificar su proyecto.Ahora continuemos. #define NFA_HCI_LAST_PROP_GATE 0xFF tNFA_HCI_DYN_GATE* nfa_hciu_alloc_gate(uint8_t gate_id, tNFA_HANDLE app_handle) { .... for (gate_id = NFA_HCI_FIRST_HOST_SPECIFIC_GENERIC_GATE; gate_id <= NFA_HCI_LAST_PROP_GATE; gate_id++) { if (gate_id == NFA_HCI_CONNECTIVITY_GATE) gate_id++; if (nfa_hciu_find_gate_by_gid(gate_id) == NULL) break; } if (gate_id > NFA_HCI_LAST_PROP_GATE) { LOG(ERROR) << StringPrintf( "nfa_hci_alloc_gate - no free Gate ID: %u " "App Handle: 0x%04x", gate_id, app_handle); return (NULL); } .... }
Advertencia de PVS-Studio: V654 CWE-834 La condición 'gate_id <= 0xFF' del bucle siempre es verdadera. nfa_hci_utils.cc 248Tenga en cuenta lo siguiente:- La constante NFA_HCI_LAST_PROP_GATE es igual a 0xFF.
- Una variable de tipo uint8_t se usa como contador de bucles . Por lo tanto, el rango de valores de esta variable es [0..0xFF].
Resulta que la condición gate_id <= NFA_HCI_LAST_PROP_GATE siempre es verdadera y no puede detener la ejecución del bucle.El analizador clasificó este error como CWE-834, pero también se puede interpretar como CWE-571: La expresión es siempre verdadera.El siguiente error en el bucle está relacionado con un comportamiento indefinido. status_t SimpleDecodingSource::doRead(....) { .... for (int retries = 0; ++retries; ) { .... }
Advertencia de PVS-Studio: V654 CWE-834 La condición '++ reintentos' del bucle siempre es verdadera. SimpleDecodingSource.cpp 226Aparentemente, el programador quería que la variable de reintentos tomara todos los valores posibles para la variable int y solo entonces el ciclo finalizó.El ciclo debería detenerse cuando la expresión ++ reintenta es 0. Y esto solo es posible si la variable se desborda. Como la variable es de tipo con signo, su desbordamiento conduce a un comportamiento indefinido. Por lo tanto, este código es incorrecto y puede tener consecuencias impredecibles. Por ejemplo, el compilador tiene todo el derecho de eliminar el cheque y dejar solo instrucciones para incrementar el contador.Y el último error en esta sección. status_t Check(const std::string& source) { .... int pass = 1; .... do { .... switch(rc) { case 0: SLOGI("Filesystem check completed OK"); return 0; case 2: SLOGE("Filesystem check failed (not a FAT filesystem)"); errno = ENODATA; return -1; case 4: if (pass++ <= 3) { SLOGW("Filesystem modified - rechecking (pass %d)", pass); continue;
Advertencia de PVS-Studio: V696 CWE-670 El operador 'continuar' terminará el ciclo 'do {...} while (FALSE)' porque la condición siempre es falsa. Verifique las líneas: 105, 121. Vfat.cpp 105Ante nosotros hay un bucle de la forma: do { .... if (x) continue; .... } while (0)
Para realizar iteraciones repetidas, el programador usa la instrucción continue . Esto esta mal.
La instrucción continue no reanuda el ciclo de inmediato, sino que continúa verificando la condición. Como la condición siempre es falsa, en cualquier caso el ciclo se ejecutará solo una vez.Para corregir el error, el código puede reescribirse, por ejemplo, así: for (;;) { .... if (x) continue; .... break; }
Reasignación Variable
Un error muy común es reescribir en una variable antes de usar el valor anterior. En la mayoría de los casos, estos errores se producen debido a un error tipográfico o un Copiar-Pegar sin éxito. Según la enumeración de debilidad común, tales errores se clasifican como CWE-563 : Asignación a variable sin uso. No sin tales errores en Android. status_t XMLNode::flatten_node(....) const { .... memset(&namespaceExt, 0, sizeof(namespaceExt)); if (mNamespacePrefix.size() > 0) { namespaceExt.prefix.index = htodl(strings.offsetForString(mNamespacePrefix)); } else { namespaceExt.prefix.index = htodl((uint32_t)-1); } namespaceExt.prefix.index = htodl(strings.offsetForString(mNamespacePrefix)); namespaceExt.uri.index = htodl(strings.offsetForString(mNamespaceUri)); .... }
Advertencia de PVS-Studio: V519 CWE-563 La variable 'namespaceExt.prefix.index' recibe valores asignados dos veces sucesivamente. Quizás esto sea un error. Verifique las líneas: 1535, 1539. XMLNode.cpp 1539Para resaltar la esencia del error, escribiré un pseudocódigo: if (a > 0) X = 1; else X = 2; X = 1;
Independientemente de la condición, a la variable X (en el código actual es namespaceExt.prefix.index ) siempre se le asignará un valor. bool AudioFlinger::RecordThread::threadLoop() { .... size_t framesToRead = mBufferSize / mFrameSize; framesToRead = min(mRsmpInFramesOA - rear, mRsmpInFramesP2 / 2); .... }
Advertencia de PVS-Studio: V519 CWE-563 A la variable 'framesToRead' se le asignan valores dos veces seguidas. Quizás esto sea un error. Verifique las líneas: 6341, 6342. Threads.cpp 6342No está claro por qué era necesario inicializar la variable al declarar, si luego se le escribe otro valor de inmediato. Algo está mal aquí. void SchedulingLatencyVisitorARM::VisitArrayGet(....) { .... if (index->IsConstant()) { last_visited_latency_ = kArmMemoryLoadLatency; } else { if (has_intermediate_address) { } else { last_visited_internal_latency_ += kArmIntegerOpLatency; } last_visited_internal_latency_ = kArmMemoryLoadLatency; } .... }
Advertencia de PVS-Studio: V519 CWE-563 La variable 'last_visited_internal_latency_' tiene valores asignados dos veces sucesivamente. Quizás esto sea un error. Verifique las líneas: 680, 682. Scheduler_arm.cc 682Código muy extraño y sin sentido. Me atrevo a sugerir que debería haber sido escrito: last_visited_internal_latency_ += kArmMemoryLoadLatency;
Y el último error, que demuestra cómo el analizador encuentra incansablemente los errores que probablemente se omitan incluso con una revisión cuidadosa del código. void multiprecision_fast_mod(uint32_t* c, uint32_t* a) { uint32_t U; uint32_t V; .... c[0] += U; V = c[0] < U; c[1] += V; V = c[1] < V; c[2] += V;
Advertencia de PVS-Studio: V519 CWE-563 A la variable 'V' se le asignan valores dos veces seguidas. Quizás esto sea un error. Verifique las líneas: 307, 309. p_256_multprecision.cc 309El código es tan "desgarrador" que no quiero entenderlo. Esto es claramente visible: hay un error tipográfico en el código, que resalté con comentarios.Otros errores
Hay errores dispersos para los cuales no tiene sentido hacer capítulos separados. Sin embargo, son tan interesantes e insidiosos como los considerados anteriormente.Operaciones prioritarias void TagMonitor::parseTagsToMonitor(String8 tagNames) { std::lock_guard<std::mutex> lock(mMonitorMutex);
Advertencia de PVS-Studio: V593 CWE-783 Considere revisar la expresión del tipo 'A = B! = C'. La expresión se calcula de la siguiente manera: 'A = (B! = C)'. TagMonitor.cpp 50Enumeración de debilidad común: CWE-783 : Error lógico de precedencia del operador.El programador concibió lo siguiente. Se busca la subcadena "3a" y la posición de esta subcadena se escribe en la variable idx . Si se encuentra la subcadena (idx! = -1), el código comienza a ejecutarse y utiliza el valor de la variable idx .Desafortunadamente, el programador está confundido acerca de las prioridades de las operaciones. De hecho, el cheque funciona así: if (ssize_t idx = (tagNames.find("3a") != -1))
Primero, verifica si hay una subcadena "3a" en la cadena, y el resultado falso / verdadero se coloca en la variable idx . Como resultado, la variable idx tiene el valor 0 o 1.Si la condición es verdadera (la variable idx es 1), la lógica que usa la variable idx comienza a ejecutarse . Una variable siempre igual a 1 conducirá a un comportamiento incorrecto del programa.Puede corregir el error si realiza la inicialización de la variable desde la condición: ssize_t idx = tagNames.find("3a"); if (idx != -1)
La nueva versión de C ++ 17 también te permite escribir: if (ssize_t idx = tagNames.find("3a"); idx != -1)
Constructor inválido struct HearingDevice { .... HearingDevice() { HearingDevice(RawAddress::kEmpty, false); } .... };
Advertencia de PVS-Studio: V603 CWE-665 El objeto se creó pero no se está utilizando. Si desea llamar al constructor, debe usar 'this-> HearingDevice :: HearingDevice (....)'. hear_aid.cc 176Enumeración de debilidad común: CWE-665 : Inicialización incorrecta.Los programadores a menudo se equivocan cuando intentan llamar explícitamente a un constructor para inicializar un objeto. Hay dos constructores en la clase. Para reducir el tamaño del código fuente, el programador decidió llamar a un constructor desde otro. Pero este código no tiene nada de lo que el desarrollador espera.Lo siguiente sucede. Se crea un nuevo objeto sin nombre de tipo HearingDevice y luego se destruye. Como resultado, los campos de clase permanecen sin inicializar.Para corregir el error, puede usar el constructor delegante (esta característica apareció en C ++ 11). El código correcto es: HearingDevice() : HearingDevice(RawAddress::kEmpty, false) { }
La función no devuelve valor int NET_RecvFrom(int s, void *buf, int len, unsigned int flags, struct sockaddr *from, int *fromlen) { socklen_t socklen = *fromlen; BLOCKING_IO_RETURN_INT( s, recvfrom(s, buf, len, flags, from, &socklen) ); *fromlen = socklen; }
Advertencia de PVS-Studio: V591 CWE-393 La función no nula debería devolver un valor. linux_close.cpp 139Enumeración de debilidad común: CWE-393 : devolución del código de estado incorrecto.La función devolverá un valor aleatorio. Otro error de este tipo: V591 CWE-393 La función no nula debería devolver un valor. linux_close.cpp 158Cálculo de tamaño de estructura incorrecto int MtpFfsHandle::handleControlRequest(....) { .... struct mtp_device_status *st = reinterpret_cast<struct mtp_device_status*>(buf.data()); st->wLength = htole16(sizeof(st)); .... }
Advertencia de PVS-Studio: V568 Es extraño que el operador 'sizeof ()' evalúe el tamaño de un puntero a una clase, pero no el tamaño del objeto de clase 'st'. MtpFfsHandle.cpp 251Estoy seguro de que querían poner el tamaño de la estructura, no el tamaño del puntero , en la variable miembro wLength . Entonces el código correcto debería ser así: st->wLength = htole16(sizeof(*st));
Respuestas similares del analizador:- V568 Es extraño que el operador 'sizeof ()' evalúe el tamaño de un puntero a una clase, pero no el tamaño del objeto de clase 'cacheinfo'. NetlinkEvent.cpp 220
- V568 Es extraño que el operador 'sizeof ()' evalúe el tamaño de un puntero a una clase, pero no el tamaño del objeto de clase 'página-> siguiente'. linker_block_allocator.cpp 146
- V568 Es extraño que el argumento del operador sizeof () sea la expresión '& session_id'. referencia-ril.c 1775
Operaciones de bits sin sentido #define EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR 0x00000001 #define EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR 0x00000002 #define EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR 0x00000004 EGLContext eglCreateContext(....) { .... case EGL_CONTEXT_FLAGS_KHR: if ((attrib_val | EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR) || (attrib_val | EGL_CONTEXT_OPENGL_FORWARD_C....) || (attrib_val | EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR)) { context_flags = attrib_val; } else { RETURN_ERROR(EGL_NO_CONTEXT,EGL_BAD_ATTRIBUTE); } .... }
Advertencia de PVS-Studio: V617 CWE-480 Considere inspeccionar la condición. El argumento '0x00000001' de '|' La operación bit a bit contiene un valor distinto de cero. egl.cpp 1329Enumeración de debilidad común: CWE-480 : uso de operador incorrecto.Una expresión de la forma (A | 1) || (A | 2) || (A | 4) no tiene sentido, ya que el resultado siempre será verdadero. De hecho, debe usar el operador & , y luego el código tiene sentido: if ((attrib_val & EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR) || (attrib_val & EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR) || (attrib_val & EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR))
Error similar: V617 CWE-480 Considere inspeccionar la condición. El argumento '0x00000001' de '|' La operación bit a bit contiene un valor distinto de cero. egl.cpp 1338Cambio de bit incorrecto template <typename AddressType> struct RegsInfo { .... uint64_t saved_reg_map = 0; AddressType saved_regs[64]; .... inline AddressType* Save(uint32_t reg) { if (reg > sizeof(saved_regs) / sizeof(AddressType)) { abort(); } saved_reg_map |= 1 << reg; saved_regs[reg] = (*regs)[reg]; return &(*regs)[reg]; } .... }
Advertencia de PVS-Studio: V629 CWE-190 Considere inspeccionar la expresión '1 << reg'. Desplazamiento de bits del valor de 32 bits con una expansión posterior al tipo de 64 bits. RegsInfo.h 47Enumeración de debilidad común: CWE-190 : Desbordamiento de enteros o envolvente.Con un desplazamiento de 1 << reg, el valor de la variable reg se encuentra en el rango [0..63]. La expresión sirve para obtener diferentes grados de dos, comenzando desde 2 ^ 0 y terminando en 2 ^ 63.El código no funciona. El hecho es que el literal numérico 1 tiene un tipo int de 32 bits . Por lo tanto, no funcionará obtener un valor mayor que 1 ^ 31. Un cambio a un valor mayor conduce al desbordamiento de la variable y al surgimiento de un comportamiento indefinido.El código correcto es: saved_reg_map |= static_cast<uint64_t>(1) << reg;
o: saved_reg_map |= 1ULL << reg;
Las cadenas se copian a sí mismas. void PCLmGenerator::writeJobTicket() {
Advertencias de PVS-Studio:- V549 CWE-688 El primer argumento de la función 'strcpy' es igual al segundo argumento. genPCLm.cpp 1181
- V549 CWE-688 El primer argumento de la función 'strcpy' es igual al segundo argumento. genPCLm.cpp 1182
De acuerdo con la clasificación de Enumeración de debilidad común: CWE-688 : Llamada de función con variable incorrecta o referencia como argumento.Las cadenas se copian por alguna razón. Lo más probable es que haya algunos errores tipográficos aquí.Usando una variable no inicializada void mca_set_cfg_by_tbl(....) { tMCA_DCB* p_dcb; const tL2CAP_FCR_OPTS* p_opt; tMCA_FCS_OPT fcs = MCA_FCS_NONE; if (p_tbl->tcid == MCA_CTRL_TCID) { p_opt = &mca_l2c_fcr_opts_def; } else { p_dcb = mca_dcb_by_hdl(p_tbl->cb_idx); if (p_dcb) { p_opt = &p_dcb->p_chnl_cfg->fcr_opt; fcs = p_dcb->p_chnl_cfg->fcs; } } memset(p_cfg, 0, sizeof(tL2CAP_CFG_INFO)); p_cfg->mtu_present = true; p_cfg->mtu = p_tbl->my_mtu; p_cfg->fcr_present = true; memcpy(&p_cfg->fcr, p_opt, sizeof(tL2CAP_FCR_OPTS));
Advertencia de PVS-Studio: V614 CWE-824 Puntero potencialmente no inicializado 'p_opt' utilizado. Considere verificar el segundo argumento real de la función 'memcpy'. mca_main.cc 252Enumeración de debilidad común: CWE-824 : Acceso del puntero no inicializado.Si p_tbl-> tcid! = MCA_CTRL_TCID y p_dcb == nullptr , entonces el puntero p_opt permanecerá sin inicializar.Uso más extraño de la variable no inicializada struct timespec { __time_t tv_sec; long int tv_nsec; }; static inline timespec NsToTimespec(int64_t ns) { timespec t; int32_t remainder; t.tv_sec = ns / kNanosPerSecond; remainder = ns % kNanosPerSecond; if (remainder < 0) { t.tv_nsec--; remainder += kNanosPerSecond; } t.tv_nsec = remainder; return t; }
Advertencia de PVS-Studio: V614 CWE-457 Variable no inicializada 't.tv_nsec' utilizada. clock_ns.h 55Enumeración de debilidad común: CWE-457 : Uso de la variable no inicializada.En el momento de disminuir la variable t.tv_nsec, no está inicializada. La variable se inicializa más tarde: t.tv_nsec = resto; .
Algo aquí está claramente confundido.Sobreexpresión void bta_dm_co_ble_io_req(....) { .... *p_auth_req = bte_appl_cfg.ble_auth_req | (bte_appl_cfg.ble_auth_req & 0x04) | ((*p_auth_req) & 0x04); .... }
Advertencia de PVS-Studio: V578 Se detectó una operación bit a bit extraña. Considera verificarlo. bta_dm_co.cc 259Esta expresión es redundante. Si elimina la subexpresión (bte_appl_cfg.ble_auth_req & 0x04) , el resultado de la expresión no cambiará. Quizás hay algún error tipográfico aquí.Manejar la fuga bool RSReflectionCpp::genEncodedBitCode() { FILE *pfin = fopen(mBitCodeFilePath.c_str(), "rb"); if (pfin == nullptr) { fprintf(stderr, "Error: could not read file %s\n", mBitCodeFilePath.c_str()); return false; } unsigned char buf[16]; int read_length; mOut.indent() << "static const unsigned char __txt[] ="; mOut.startBlock(); while ((read_length = fread(buf, 1, sizeof(buf), pfin)) > 0) { mOut.indent(); for (int i = 0; i < read_length; i++) { char buf2[16]; snprintf(buf2, sizeof(buf2), "0x%02x,", buf[i]); mOut << buf2; } mOut << "\n"; } mOut.endBlock(true); mOut << "\n"; return true; }
Advertencia de PVS-Studio: V773 CWE-401 La función se cerró sin liberar el controlador 'pfin'. Una fuga de recursos es posible.
slang_rs_reflection_cpp.cpp 448El analizador clasificó este error según la Enumeración de debilidad común como: CWE-401 : Liberación incorrecta de memoria antes de eliminar la última referencia ('Fuga de memoria'). Sin embargo, sería más correcto aquí emitir CWE-775 : Falta la publicación del descriptor de archivo o el identificador después de la vigencia de la vida útil. Instruiré a mis colegas para que corrijan este defecto en PVS-Studio.El mango del pfin no se libera en ningún lado. Simplemente olvidé llamar a la función fclose al final . Un error desagradable que puede agotar rápidamente todo el suministro de descriptores disponibles, después de lo cual será imposible abrir nuevos archivos.Conclusión
Como puede ver, incluso en un proyecto tan conocido y probado como Android, el analizador PVS-Studio encuentra fácilmente muchos errores y vulnerabilidades potenciales. Para resumir qué debilidades (vulnerabilidades potenciales) se encontraron:- CWE-14: Eliminación del compilador de código para borrar buffers
- CWE-20: Validación de entrada incorrecta
- CWE-119: Restricción incorrecta de operaciones dentro de los límites de un búfer de memoria
- CWE-190: Desbordamiento de enteros o envolvente
- CWE-198: Uso de orden de bytes incorrecto
- CWE-393: Devolución del código de estado incorrecto
- CWE-401: Liberación incorrecta de memoria antes de eliminar la última referencia ('Fuga de memoria')
- CWE-457: Uso de variable no inicializada
- CWE-462: Clave duplicada en lista asociativa
- CWE-480: uso de operador incorrecto
- CWE-484: Declaración de interrupción omitida en el interruptor
- CWE-561: Código muerto
- CWE-562: Devolución de dirección variable de pila
- CWE-563: Assignment to Variable without Use
- CWE-570: Expression is Always False
- CWE-571: Expression is Always True
- CWE-476: NULL Pointer Dereference
- CWE-628: Function Call with Incorrectly Specified Arguments
- CWE-665: Improper Initialization
- CWE-670: Always-Incorrect Control Flow Implementation
- CWE-682: Incorrect Calculation
- CWE-688: Function Call With Incorrect Variable or Reference as Argument
- CWE-690: Unchecked Return Value to NULL Pointer Dereference
- CWE-691: Insufficient Control Flow Management
- CWE-758: Reliance on Undefined, Unspecified, or Implementation-Defined Behavior
- CWE-762: Mismatched Memory Management Routines
- CWE-775: Missing Release of File Descriptor or Handle after Effective Lifetime
- CWE-783: Operator Precedence Logic Error
- CWE-824: Access of Uninitialized Pointer
- CWE-834: Excessive Iteration
En total, describí 490 vulnerabilidades potenciales en el artículo. De hecho, el analizador puede identificar más de ellos, pero, como escribí anteriormente, no encontré la fuerza para estudiar el informe con más cuidado.El tamaño de la base de código probada es de aproximadamente 2,168,000 líneas de código en C y C ++. De estos, el 14,4% son comentarios. En total, obtenemos alrededor de 1,855,000 líneas de código limpio.Por lo tanto, tenemos 490 CWE para 1,855,000 líneas de código.Resulta que el analizador PVS-Studio puede detectar más de 1 debilidad (vulnerabilidad potencial) por cada 4000 líneas de código en un proyecto de Android. Buen resultado para un analizador de código, me alegro.Gracias por su atencion!
Les deseo a todos un código sin código y propongo hacer lo siguiente:- Descargue PVS-Studio y verifique el borrador de trabajo.
- : : .
- , : twitter , RSS , vk.com .
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Andrey Karpov.
We Checked the Android Source Codes by PVS-Studio or Nothing is Perfect