Wireshark Foundation lanzó la versión estable final del popular analizador de tráfico de red: Wireshark 3.0.0. La nueva versión corrige varios errores, ahora es posible analizar los nuevos protocolos, además de que se reemplaza el controlador en Npcap WinPcap. Aquí es donde termina la cita del anuncio y comienza nuestra nota sobre errores en el proyecto. Los autores del proyecto definitivamente no han hecho todo lo posible para corregir errores antes del lanzamiento.
Recopilemos las revisiones ahora para dar un motivo al 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 intuitiva y lógica, un sistema todopoderoso de filtros. Wireshark es multiplataforma, funciona en sistemas operativos como Windows, Linux, macOS, Solaris, FreeBSD, NetBSD y muchos otros.
Para hacer el análisis del código fuente, utilizamos el analizador de código estático
PVS-Studio . Para analizar el código fuente, primero necesitábamos compilar el proyecto en un sistema operativo. La elección fue amplia no solo por la naturaleza multiplataforma del proyecto, sino también por la del analizador. Elegí macOS para el análisis. También puede ejecutar el analizador en Windows y Linux.
Me gustaría llamar la atención sobre la calidad del código. Desafortunadamente, no puedo darle grandes puntos. Es una evaluación subjetiva, pero como revisamos regularmente muchos proyectos, tengo un marco de referencia. Lo que se destaca en este caso es una gran cantidad de advertencias de PVS-Studio para una pequeña cantidad de código. En total, se activaron más de 3500 advertencias de todos los niveles para este proyecto. Es típico de los proyectos, que generalmente no utilizan herramientas de análisis estático, incluso gratuitas. Otro factor que apunta a la calidad del proyecto son los errores repetidos detectados por el analizador. No citaré ejemplos de código del mismo tipo, mientras que algunos errores similares ocurren en cientos de lugares.
Estos insertos tampoco aumentan la calidad del código:
#line 1 "./asn1/acse/packet-acse-template.c"
Hay más de 1000 de ellos en todo el proyecto. Tales insertos hacen que sea más difícil para el analizador hacer coincidir las advertencias emitidas con los archivos apropiados. Bueno, creo que a los desarrolladores promedio no les gustará mantener ese 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 estructuras de dos tipos:
mate_cfg_gog y
mate_cfg_gop, son muy similares, pero no iguales. Lo más probable es que, en este fragmento de código, las funciones estén mezcladas, lo que está lleno de posibles errores en el programa al acceder a la memoria mediante un puntero.
Aquí están los fragmentos de estructuras de datos mezcladas:
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, se reescribe el valor (que se acaba de evaluar) de la variable
HDR_TCP.dest_port .
Errores lógicos
En esta sección, citaré varios ejemplos de errores en operadores condicionales, y todos serán completamente 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. De acuerdo con las expresiones escritas con el operador AND, al llegar a la condición interna, el valor de la
dirección variable definitivamente será diferente de 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 solo de una 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 del operador 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. No es el error real, pero el código redundante dificulta la lectura del código y la 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 redundante, probablemente con un error tipográfico, y otra cosa debe verificarse en una de las condiciones del bloque
if .
Extraño afirma
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);
La verificación del puntero
g_assert es redundante aquí, ya que el puntero ya se verificó antes de eso. Tal vez, solo
g_assert estaba en esta función y un desarrollador olvidó eliminarlo, pero tal vez un campo de estructura debería haberse verificado aquí.
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á muy claro por qué la
aserción , que duplica la condición del bucle, tiene lugar en la función. El contador de bucles no cambiará en el cuerpo.
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 unas pocas líneas antes de comprobar si es nulo.
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. Cuando se maneja un caso especial en el programa, esta matriz primero se borra mediante la función
g_strfreev y luego se usa una cadena de esta matriz en el mensaje de error. Lo más probable es que estas líneas 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 , necesitamos llamar a la función
g_free en algún momento. No se realiza en el fragmento de código dado y se asigna una nueva parte de la memoria en el bucle en cada iteración. Aquí vienen múltiples fugas de memoria.
Algunas otras advertencias 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, en el código hay muchos otros casos similares, donde 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, incluso haciéndolo varias veces. No podemos decir con certeza si es un error o no, sin embargo, hay alrededor de 10 de estos bucles 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 , tomado por la función, se cambia inmediatamente con otro valor. Es muy sospechoso Además, el código contiene varias docenas de esos lugares, por lo que es difícil decidir si es un error o no. Me encontré con un código similar en otro gran proyecto, ese código era correcto allí, nadie simplemente 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 ha detectado la sobrecarga no válida de la función
headerData . Las funciones tienen diferentes valores predeterminados del parámetro de
rol . Esto puede causar un comportamiento incorrecto, no el esperado por un programador.
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 lenguaje.
Lo más probable es que el código correcto sea así:
if (bitshift == 64) available_bits = 0; else available_bits = G_GUINT64_CONSTANT(0xFFFFFFFFFFFFFFFF) >> bitshift;
Conclusión
Puede parecer que esta revisión muestra pocos errores, pero en el informe completo los casos considerados se repiten docenas y cientos de veces. Además, las revisiones de advertencias de PVS-Studio son de naturaleza demostrativa. Representan una contribución a la calidad de los proyectos de código abierto, pero los controles únicos son los más ineficientes en términos de 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.