La Fundación Wireshark ha lanzado la última versión estable del popular analizador de tráfico de red: Wireshark 3.0.0. La nueva versión corrigió varios errores, implementó la capacidad de analizar nuevos protocolos y reemplazó el controlador WinPcap con Npcap. Aquí termina la cita del anuncio y comienza nuestro artículo sobre errores en el proyecto. Antes del lanzamiento, se corrigieron claramente de manera insuficiente. Veamos algunas correcciones para que haya una razón para hacer una nueva versión :).
Introduccion
Wireshark es una herramienta bien conocida para capturar y analizar el tráfico de red. El programa funciona con la gran mayoría de los protocolos conocidos, tiene una interfaz gráfica clara y lógica, un potente sistema de filtro. Wireshark: multiplataforma, funciona en sistemas operativos como: Windows, Linux, macOS, Solaris, FreeBSD, NetBSD y muchos otros.
Para buscar errores, utilizamos el analizador estático
PVS-Studio . Para analizar el código fuente, primero debe compilar el proyecto en algún sistema operativo. La elección fue excelente no solo por la naturaleza multiplataforma del proyecto, sino también por la naturaleza multiplataforma del analizador. Para el análisis del proyecto, elegí macOS. El analizador también se puede iniciar en Windows y Linux.
Me gustaría hablar sobre la calidad del código por separado. Lamentablemente, no puedo llamarlo bueno. Esta es una evaluación subjetiva, pero como revisamos regularmente muchos proyectos, tengo algo con lo que comparar. En este caso, una gran cantidad de advertencias de PVS-Studio en una pequeña cantidad de código son sorprendentes. En total, se han emitido más de 3.500 advertencias de todos los niveles para este proyecto. Esto es típico para proyectos que no utilizan herramientas de análisis estático, incluso las gratuitas. Otro factor que indica la calidad del proyecto son los errores repetidos identificados por el analizador. Los mismos ejemplos de código no se darán en el artículo, pero algunos errores idénticos están presentes en el código en cientos de lugares.
Además, estos insertos no agregan calidad al código:
#line 1 "./asn1/acse/packet-acse-template.c"
Hay más de 1000 de ellos en todo el proyecto. Tales insertos dificultan que el analizador compare las advertencias generadas con el archivo deseado. Pero estoy seguro de que los desarrolladores comunes no disfrutan del soporte de dicho código.
Errores tipográficos
Advertencia 1V641 El tamaño del búfer de memoria asignado no es un múltiplo del tamaño del elemento. mate_setup.c 100
extern mate_cfg_gog* new_gogcfg(mate_config* mc, gchar* name) { mate_cfg_gog* cfg = (mate_cfg_gog *)g_malloc(sizeof(mate_cfg_gop)); .... }
Hay dos tipos de estructuras:
mate_cfg_gog y
mate_cfg_gop , son muy similares, pero no idénticas. Lo más probable es que las funciones se mezclaron en este fragmento de código, que está plagado de posibles errores en el programa durante el acceso a la memoria por puntero.
Los siguientes son fragmentos de estructuras de datos confusas:
typedef struct _mate_cfg_gog { gchar* name; GHashTable* items; guint last_id; GPtrArray* transforms; LoAL* keys; AVPL* extra; float expiration; gop_tree_mode_t gop_tree_mode; gboolean show_times; .... } mate_cfg_gog; typedef struct _mate_cfg_gop { gchar* name; guint last_id; GHashTable* items; GPtrArray* transforms; gchar* on_pdu; AVPL* key; AVPL* start; AVPL* stop; AVPL* extra; float expiration; float idle_timeout; float lifetime; gboolean drop_unassigned; gop_pdu_tree_t pdu_tree_mode; gboolean show_times; .... } mate_cfg_gop;
Advertencia 2V519 La variable 'HDR_TCP.dest_port' tiene valores asignados dos veces sucesivamente. Quizás esto sea un error. Líneas de verificación: 495, 496. text_import.c 496
void write_current_packet (void) { .... HDR_TCP.source_port =isOutbound ? g_htons(hdr_dest_port):g_htons(hdr_src_port); HDR_TCP.dest_port = isOutbound ? g_htons(hdr_src_port) :g_htons(hdr_dest_port); HDR_TCP.dest_port = g_htons(hdr_dest_port); .... }
En la última línea, el valor calculado de la variable
HDR_TCP.dest_port no está
confirmado .
Errores lógicos
En esta sección daré algunos ejemplos de errores en las declaraciones condicionales, y todos ellos serán fundamentalmente diferentes entre sí.
Advertencia 1V547 La expresión 'direction == 0' siempre es falsa. paquete-adb.c 291
#define P2P_DIR_RECV 1 #define P2P_DIR_SENT 0 static void save_command(....) { .... if ( service_data && service_data->remote_id == 0 && direction == P2P_DIR_RECV) { if (direction == P2P_DIR_SENT) { service_data->remote_id = arg1;
En la condición externa, la variable de
dirección se compara con la constante
P2P_DIR_RECV . Escribir expresiones a través del operador AND significa que cuando se alcanza la condición interna, el valor de la variable de
dirección no será exactamente igual a la otra constante
P2P_DIR_SENT .
Advertencia 2V590 Considere inspeccionar el '(tipo == 0x1) || (tipo! = 0x4) 'expresión. La expresión es excesiva o contiene un error de imprenta. paquete-fcsb3.c 686
static int dissect_fc_sbccs (....) { .... else if ((type == FC_SBCCS_IU_CMD_HDR) || (type != FC_SBCCS_IU_CMD_DATA)) { .... }
El error de este fragmento de código es que el resultado de la condición depende de una sola expresión:
(type != FC_SBCCS_IU_CMD_DATA)
Advertencia 3V590 Considere inspeccionar esta expresión. La expresión es excesiva o contiene un error de imprenta. snort-config.c 40
static char *skipWhiteSpace(char *source, int *accumulated_offset) { int offset = 0; while (source[offset] != '\0' && source[offset] == ' ') { offset++; } *accumulated_offset += offset; return source + offset; }
El resultado de la declaración condicional dependerá solo de esta parte de la expresión
(fuente [desplazamiento] == '') . La comprobación
(fuente [desplazamiento]! = '\ 0') es redundante y se puede eliminar de forma segura. Esto no es un error real, pero el código redundante dificulta la lectura y comprensión del programa, por lo que es mejor simplificarlo.
Advertencia 4V547 La expresión 'eras_pos! = NULL' siempre es verdadera. reedsolomon.c 659
int eras_dec_rs(dtype data[NN], int eras_pos[NN-KK], int no_eras) { .... if(eras_pos != NULL){ for(i=0;i<count;i++){ if(eras_pos!= NULL) eras_pos[i] = INDEX_TO_POS(loc[i]); } } .... }
Quizás estamos lidiando con una verificación innecesaria, y posiblemente con un error tipográfico, y en uno de los ifs, algo más debería verificarse.
Activos extraños
Advertencia 1V547 La expresión 'sub_dissectors! = NULL' siempre es verdadera. capture_dissectors.c 129
void capture_dissector_add_uint(....) { .... sub_dissectors = (struct capture_dissector_table*)g_hash_table_lookup(....); if (sub_dissectors == NULL) { fprintf(stderr, "OOPS: Subdissector \"%s\" not found ... \n", name); if (getenv("WIRESHARK_ABORT_ON_DISSECTOR_BUG") != NULL) abort(); return; } g_assert(sub_dissectors != NULL);
Verificar el puntero en
g_assert en este punto es superfluo, ya que el puntero se verifica antes de eso. Quizás en esta función solo había
g_assert antes , y se olvidaron de eliminarlo, pero tal vez aquí debería verificar algún campo de estructura.
Advertencia 2V547 La expresión 'i <count' siempre es verdadera. packet-netflow.c 10363
static int dissect_v9_v10_template_fields(....) { .... count = tmplt_p->field_count[fields_type]; for(i=0; i<count; i++) { .... if (tmplt_p->fields_p[fields_type] != NULL) { DISSECTOR_ASSERT (i < count);
No está del todo claro por qué una
aserción está presente en la función, que duplica la condición del bucle. El contador de ciclos en el cuerpo no cambia.
Errores con punteros
Advertencia 1V595 El puntero 'si-> conv' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 2135, 2144. packet-smb2.c 2135
static int dissect_smb2_fid(....) { .... g_hash_table_insert(si->conv->fids, sfi, sfi);
El puntero
si-> conv se desreferencia varias líneas antes de que se verifique si es igual a cero o no.
Advertencia 2V774 El puntero 'protos' se usó después de liberar la memoria. paquete-k12.c 311
static gboolean k12_update_cb(void* r, char** err) { gchar** protos; .... for (i = 0; i < num_protos; i++) { if ( ! (h->handles[i] = find_dissector(protos[i])) ) { h->handles[i] = data_handle; h->handles[i+1] = NULL; g_strfreev(protos); *err = g_strdup_printf("Could not find dissector for: '%s'", protos[i]); return FALSE; } } .... }
protos es una serie de cadenas. Durante el procesamiento de una excepción en el programa, esta matriz primero se borra mediante la función
g_strfreev , y luego se usa una de las líneas de esta matriz en el mensaje de error. Lo más probable es que estas líneas en el código se intercambien:
*err = g_strdup_printf("Could not find dissector for: '%s'", protos[i]); g_strfreev(protos);
Fugas de memoria
V773 Al puntero 'ptmpstr' se le asignaron valores dos veces sin liberar la memoria. Una pérdida de memoria es posible. idl2wrs.c 2436
static void parsetypedefunion(int pass) { char tmpstr[BASE_BUFFER_SIZE], *ptmpstr; .... while(num_pointers--){ g_snprintf(tmpstr, BASE_BUFFER_SIZE, "%s_%s", ptmpstr, "unique"); FPRINTF(eth_code, "static int\n"); FPRINTF(eth_code, "....", tmpstr); FPRINTF(eth_code, "{\n"); FPRINTF(eth_code, " ....", ptmpstr, ti->str); FPRINTF(eth_code, " return offset;\n"); FPRINTF(eth_code, "}\n"); FPRINTF(eth_code, "\n"); ptmpstr=g_strdup(tmpstr); } .... }
Después de la función
g_strdup , debe llamar a la función
g_free en algún momento. En el fragmento de código presentado, esto no se hace, y en el ciclo en cada iteración se asigna una nueva pieza de RAM. Se producen múltiples pérdidas de memoria.
Algunas advertencias más para fragmentos de código similares:
- V773 Al puntero 'ptmpstr' se le asignaron valores dos veces sin liberar la memoria. Una pérdida de memoria es posible. idl2wrs.c 2447
- V773 Al puntero 'ptmpstr' se le asignaron valores dos veces sin liberar la memoria. Una pérdida de memoria es posible. idl2wrs.c 2713
- V773 Al puntero 'ptmpstr' se le asignaron valores dos veces sin liberar la memoria. Una pérdida de memoria es posible. idl2wrs.c 2728
- V773 Al puntero 'ptmpstr' se le asignaron valores dos veces sin liberar la memoria. Una pérdida de memoria es posible. idl2wrs.c 2732
- V773 Al puntero 'ptmpstr' se le asignaron valores dos veces sin liberar la memoria. Una pérdida de memoria es posible. idl2wrs.c 2745
Desafortunadamente, hay muchos otros lugares similares en el código donde no se libera memoria.
Misceláneo
Advertencia 1V535 La variable 'i' se está utilizando para este bucle y para el bucle externo. Líneas de verificación: 7716, 7798. packet-opa-mad.c 7798
static gint parse_GetVFInfo(....) { .... for (i = 0; i < records; i++) {
En una función muy larga, los desarrolladores cambian audazmente el valor del contador de bucle y lo hacen varias veces. Es difícil decir si esto es un error o no, pero hay unos 10 ciclos de este tipo en el proyecto.
Advertencia 2El 'elemento' del parámetro V763 siempre se reescribe en el cuerpo de la función antes de ser utilizado. paquete-cdma2k.c 1324
static void cdma2k_message_ORDER_IND(proto_item *item, ....) { guint16 addRecLen = -1, ordq = -1, rejectedtype = -1; guint16 l_offset = -1, rsc_mode_ind = -1, ordertype = -1; proto_tree *subtree = NULL, *subtree1 = NULL; item = proto_tree_add_item(tree,hf_cdma2k_OrderIndMsg, tvb, ....);
El puntero del
elemento que toma la función se deshilacha inmediatamente por otro valor. Esto es muy sospechoso. Además, el código contiene varias docenas de dichos lugares, por lo que es difícil decir si esto es un error o no. Conocí un código similar anteriormente en otro gran proyecto, allí estaba el código correcto, pero nadie se atrevió a cambiar la interfaz de la función.
Advertencia 3V762 Es posible que una función virtual se haya anulado incorrectamente. Vea el tercer argumento de la función 'headerData' en la clase derivada 'PacketListModel' y la clase base 'QAbstractItemModel'. packet_list_model.h 48
QVariant QAbstractItemModel::headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const
El analizador detectó una sobrecarga incorrecta de la función
headerData . Las funciones tienen un valor predeterminado diferente para el parámetro de
rol . Esto puede no conducir al comportamiento que el programador esperaba.
Advertencia 4V610 Comportamiento indefinido. Verifique el operador de turno '>>'. El operando derecho ('bithift' = [0..64]) es mayor o igual que la longitud en bits del operando izquierdo promovido. proto.c 10941
static gboolean proto_item_add_bitmask_tree(...., const int len, ....) { .... if (len < 0 || len > 8) g_assert_not_reached(); bitshift = (8 - (guint)len)*8; available_bits = G_GUINT64_CONSTANT(0xFFFFFFFFFFFFFFFF) >> bitshift; .... }
Un cambio de 64 bits dará como resultado un comportamiento indefinido de acuerdo con el estándar del idioma.
Más bien, el código correcto debería ser así:
if (bitshift == 64) available_bits = 0; else available_bits = G_GUINT64_CONSTANT(0xFFFFFFFFFFFFFFFF) >> bitshift;
Conclusión
Puede parecer que hay pocos ejemplos de errores en la revisión, pero en el informe completo los casos presentados se repiten docenas y cientos de veces. Las descripciones generales de advertencia de PVS-Studio son solo para fines de demostración. Esta es una contribución a la calidad de los proyectos de código abierto, pero los controles únicos son la forma más ineficiente de aplicar la metodología de análisis estático.
Puede obtener y analizar el informe completo usted mismo. Para hacer esto, solo necesita
descargar y ejecutar el analizador PVS-Studio.
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Svyatoslav Razmyslov.
Wireshark 3.x: análisis de código bajo macOS y revisión de errores