[IRC-DEV] De los errores de aprende (*->name)

Tue Nov 18 18:03:21 CET 2003

Se están migrando varios campos de cadena de tamaño fijo de las
estructuras "Client" a campos de tamaño dinámico, con vistas al futuro
"slab allocator".

El primer paso ha sido migrar el campo "name" de HOSTLEN+1 (65)
caracteres a un "malloc" externo. La motivación de ello es que en muy
raras ocasiones se utilizan los 65 caracteres al completo, y hacer un
"malloc" separado resulta rentable aún a pesar de su "sobrecarga" de
memoria. Con el SLAB allocator la diferencia será aún mayor.

El parche original es el u2.10.H.08.79. Contiene varios bugs.

El primer bug es que *NO* se migraron todas las asignaciones de
"*->name" de una cadena fija a memoria dinámica. Eso se puede ver con
facilidad en
<http://devel.irc-hispano.org/bin/cvsweb/viewcvs.cgi/ircd/s_serv.c.diff?r1=1.79&r2=1.82>.

Hay que comprobar que no nos dejamos más asignaciones. Para ello vemos
que las asignaciones originales solo pueden ser "memcpy", "strcpy" o
"strncpy". Así, buscamos:

* "memcpy.*->name.*,":

 Encontramos dos casos, en "s_bsd.c" y "s_serv.c". Ambas son copias
sobre la memoria dinámica, como debe ser.

* "strcpy.*->name.*,":

 Encontramos 16 casos. Uno en "hash.c", tres en "ircd.c", uno en
"res.c", tres en "s_bsd.c", uno en "s_ping.c" y siete en "s_user.c".

 Todos son copias sobre la memoria dinámica, como debe ser.

* "strncpy.*->name.*,":

 Esta búsqueda hubiera detectado el caso parcheado en u2.10.H.08.81.

 Se detectan casos en "s_conf.c", "s_debug.c", "s_serv.c" y "s_user.c".

 Los casos de "s_conf.c", "s_user.c" y "s_debug.c" no se corresponden
con "->name" (falsos positivos).

 El caso de "s_serv.c" es copia sobre la memoria dinámica, como debe
ser.

Con estas búsquedas estamos razonablemente seguros de no haber dejado
"cabos sueltos" en lo que la asignación de memoria se refiere.

Otro problema del parche u2.10.H.08.79 original, solucionado en
u2.10.H.08.82, es el uso de memoria posiblemente dinámica para el campo
"parv[0]" de los comandos IRC. Ello no sería problema si no fuera porque
algunos comandos pueden liberar dicha memoria mientras está siendo usada
como "parv[0]", ocasionando fallos esporádicos.

La solución más obvia sería no liberar dicha memoria, pero ello
obligaria a liberarla de forma "diferida". Otra opción, adoptada por el
parche u2.10.H.08.82, es tener una memoria "estática" para "parv[0]" y
realizar la copia del campo "->name" correspondiente. El impacto sobre
el rendimiento debería ser inapreciable. Esta vía pueede solucionar,
también (en el futuro), el chanchullo de "not_sender".

Se trata de un problema "conceptual" que se descubre conociendo el
código y su funcionamiento. No es algo que se pueda automatizar.

Un tercer bug de u2.10.H.08.79 permitía matar el servidor de IRC al
hacer un "ghost" durante la conexión inicial de un cliente. Puede verse
con claridad en
<http://devel.irc-hispano.org/bin/cvsweb/viewcvs.cgi/ircd/s_user.c.diff?r1=1.242&r2=1.243>

Aquí puede verse como se llama a una función ("sendto_one") con un
parámetro que puede ser NULL. Como dicha función acaba llamando a
"sprintf_irc", debe gestionarse este caso. La solución es simple, como
se ve en la URL anterior.

Para ver si existen casos similares no descubiertos aún, hacemos uhna
búsqueda por "sendto_.*->name". Nos salen 93 casos, que hay que analizar
uno por uno.

Las funciones involucradas son: "find_chasing", "add_banid",
"set_mode_local", "set_mode_remoto", "list_next_channels", "m_create",
"m_kick", "m_invite", "m_list", "report_classes", "try_connections",
"check_pings", "dump_map", "chequea_estado_watch",
"muestra_estado_watch", "m_watch", "report_configured_links", "m_stats",
"parse_server", "m_admin", "timeout_query_list", "m_dns", "db_die",
"inetport", "completed_connection", "read_message", "connect_inet",
"connect_unix", "find_kill", "dead_link", "sendbufto_one",
"exit_client", "m_server", "m_error", "m_end_of_burst",
"m_end_of_burst_ack", "hunt_server", "register_user", "m_message",
"whisper", "send_umode_out", "m_umode", "is_silenced", "add_silence",
"m_silence", "make_virtualhost", "rename_user", "m_nick_local" y
"calc_load".

En todas esas funciones hay que comprobar que el uso que se hace sea
"apropiado". Tarea larga y tediosa, pero trivial.

Las siguientes funciones parecen especialmente delicadas: "check_pings".
Hay que vérsela con calma y buena letra.

El último problema parece ser un "memory leak", ya que constato que el
IRCD ocupa cada vez más memoria, de forma consistente. Dado que solo hay
un bloque de memoria dinámica por cada cliente (sea servidor o usuario),
hay dos posibles maneras de que se produzca el "memory leak":

* No se libera la memoria correctamente cuando se libera el "cliente".

 Para ello buscamos donde se libera una estructura "sptr", "cptr" o
"acptr", que son los nombres normales.

Aparecen tres liberaciones de "RunFree(cptr)". Una en "list.c" y dos en
"s_bsd.c".

Las dos de "s_bsd.c" son espureas (comentarios), así que solo queda
"list.c". Su liberación se produce en la función "free_client", que es
llamada desde "list.c", "numnicks.c", "s_ping.c" y "s_bsd.c".c

Investigando tenemos nuevas funciones: "remove_client_from_list",
"add_listener", "end_ping", etc.

Aunque todo parece correcto, es mucho más lógico liberar "cptr->name" en
el mismo sitio que se libera "cptr". Es decir, en "free_client". Y así
lo hago, en el parche u2.10.H.08.85. De esta forma evitamos dejarnos
nada en el tintero.

* No se libera la memoria correctamente cuando se hace un cambio de
"name".

 Hay que comprobar que todo "RunMalloc" debe estar acompañado de un
"RunFree" previo, para liberar la posible memoria anterior. Para ello
simplemente buscamos ">name.*RunMalloc", y revisamos caso por caso.

No se ve ningún caso especial.

Asi pues, tiene pinta de que el "memory leak" se debe a que no se libera
correctamente el "name" cuando se corta una conexión con un usuario. Con
el cambio hecho en u2.10.H.08.85 debería solucionarse ese problema.
Lanzo un servidor de IRC actualizado y comprobaré su memoria usada
dentro de unos días, para verificar si se sigue produciendo el "leak".

-- 
Jesus Cea Avion                         _/_/      _/_/_/        _/_/_/
jcea at argo.es http://www.argo.es/~jcea/ _/_/    _/_/  _/_/    _/_/  _/_/
                                      _/_/    _/_/          _/_/_/_/_/
PGP Key Available at KeyServ   _/_/  _/_/    _/_/          _/_/  _/_/
"Things are not so easy"      _/_/  _/_/    _/_/  _/_/    _/_/  _/_/
"My name is Dump, Core Dump"   _/_/_/        _/_/_/      _/_/  _/_/
"El amor es poner tu felicidad en la felicidad de otro" - Leibniz