sábado, 22 de enero de 2011

News: El Bug navideño en el MM

bug
 Hoy es 22 de Diciembre del 2010. Son las 11 de la noche, lleva lloviendo dos días casi sin parar y apetece quedarse en casa calentito. En la tele siguen echando la misma basurilla de siempre, me acoplo en el sofá,arranco el ordenador y me pongo a testear por enésima vez ReactOS.

“La televisión ha hecho maravillas por mi cultura. En cuanto alguien enciende la televisión, voy a la biblioteca y me leo un buen libro.” dijo Groucho Marx , “o me pongo a testear ReactOS” añadiría yo.



Me bajo la última versión disponible, la 50109, y comienzo a instalarla…De pronto la instalación se congela al 25% y de ahí no avanza. "No pasa nada"-digo,mientras una gota fría comienza a resbalar por la patilla....

 

 

Instalando ReactOS.

 


system-install-1 La instalación comienza de nuevo, esta vez se congela al 45%.¿Pero qué !%$@#….? Reiniciemos otra vez…
Re-re-comienza la instalación y de nuevo se congela al 45%. Esto ya no es normal.

Por un azar del destino tengo instalada la 50085,que es del día anterior(21 de Diciembre).Instalé esa revisión porque incluye un pequeño parche que había creado para arreglar un buffer overrun en el driver de vídeo y quería comprobar que todo funcionara correctamente.

Recordaba precisamente que la instalación de la 50085 había ido como la seda pues estaba atento a cualquier “problemilla” que pudiera aparecer.
Así pues entre la 50109 y la 50085 un bug estaba impidiendo que ReactOS pudiera instalarse.



Analizando el problema…



Expirar,Inspirar…Expirar,Inspirar…Un bug que destroza la instalación del Sistema Operativo es considerado crítico…y hay que encontrarlo cuanto antes.
Mi madre sentada a mi lado,ve mi cara congestionada.”¿Estás bien?”me pregunta.

byte_lupa
Hay dos manera de testear. Una es ir probando revisiones hasta encontrar cual introdujo el bug. Otra es analizar las revisiones y descartar a priori cuales no son susceptibles de introducir el bug. A medida que te vas familiarizando con los módulos de ReactOS es muy ¿fácil? ir descartando revisiones.

Asi pues, primero analizas el problema: “ReactOS se cuelga al copiar sus ficheros en el disco duro”.


La instalación de ReactOS es muy simple y tiene “dos partes”. En la “primera” se copian los archivos fundamentales al disco duro para controlar la “segunda parte” .Esta “primera” parte es muy rápida y ocurre antes de seleccionar el idioma de instalación. La “segunda” parte consiste básicamente en extraer los ficheros de un fichero comprimido llamado “reactos.cab” y copiarlos al disco duro.

El problema está apareciendo a la hora de “descomprimir” y/o “almacenar en memoria”.
“Sí,ahora ceno, un segundo”, le contesto.





Analizando las revisiones

 

 

Por fortuna el rango es muy pequeño 50085-50109,solo 24, veamos cuáles se pueden descartar. Aquí puedes encontrar un log de todas nuestras revisiones:GIT:

50089: [SCSIPORT] //Googleando vemos que es un driver para interconectar ordenadores.No necesitamos este driver para llevar a cabo la segunda parte de la instalación .Descartado.

50091/50093: [FORMATTING] //Como bien dice, es solo una recolocación de formato del código( colocar bien los espacios y demás).Comprobamos que no ha eliminado otras cosas (es típico que se te vaya el dedo y te comas un “{“).Correcto.Descartado,

50100: [CMAKE]//Cmake es un módulo en progreso que no es básico.Descartado

50103: [WIN32K]Silence DebugPrints//Esto solo elimina información mostrada en el DebugLog, es algo trivial.Correcto.Descartado.

50105:[I8042PRT]//googleando te das cuenta que es un driver para controlar el ratón. En esta fase de la instalación solo se utiliza el teclado.La “primera fase” es tan básica que ni se cargan los controladores del ratón. Descartado

Además las revisiones: 50088,50094,50097,50101,50102,50104 no existen (pues se tratan de parches aplicados a otras ramas, como puede ser Arwinss, o Cmake) y por tanto no pueden afectar al comportamiento del trunk.

Hemos descartado por tanto 12 revisiones. Solo quedan 12 candidatas.(Qué optimista!)

El gap 50085-50109 vamos a dividirlo en dos: 50085(Funciona)-50099(¿funcionará?) y 50099(¿funcionará?)-50109(No funciona).

Hay un interesante GAP, el que va de la 50099 a la 50109.

En este GAP hay 10 revisiones, de las que dos son inexistentes(50101 y 50102), 2 que ya han sido descartadas y 6 candidatas.Es interesante probar por tanto si la 50099 funcionaba o ya estaba rota. Si está rota sabemos que el commit culpable está entre la 50085 y la 50099. Si la 50099 funciona entonces el problema está en el segundo GAP el que va de la 50099 a la 50109.

Descargamos la 50099 y vemos que funciona perfecta, sin problemas a la hora de instalarse. Ya sabemos cual es el GAP donde hemos introducido el bug: 50099-50109




Analizando el GAP.

 


image

Realmente estaba teniendo mala suerte, no es normal encontrar tantos cambios críticos en el [NTOS]kernel.Y en este GAP había ni más ni menos que 6. Había dos opciones, testear las 6 o revisar el código por si podía encontrar algún bug evidente…




Al menos ,con un rápido chequeo ,se habían reducido las posibilidades de 24 a 6.No hay mal que por bien no venga.

Por mirar que no quede, a si que me propuse revisar los 6 commits sabiendo de antemano que seguramente no entendería ni una línea de código..

¿Por donde empiezo a mirar los cambios?¿De 50099 a 50109 ?¿o en el sentido inverso?

Siempre me han dicho que las casas nunca se construyen por el tejado.

Siempre me ha gustado llevar la contraria:Pruebo de mayor a menor.Venga: la 50108.





La 50108 y la buena suerte… 


Parece un commit sencillito, de él 3 líneas son comentarios de código y sólo 2 es código añadido.De haber un bug tiene que estar en el nuevo código añadido(lo marco en verde):

--- a/reactos/ntoskrnl/mm/ARM3/virtual.c
+++ b/reactos/ntoskrnl/mm/ARM3/virtual.c
@@ -2446,6 +2446,9 @@ MiQueryMemoryBasicInformation(IN HANDLE ProcessHandle,
             /* Check if this VAD is too high */
             if (BaseVpn < Vad->StartingVpn)
             {
+                /* Stop if there is no left child */
+                if (!Vad->LeftChild) break;
+
                 /* Search on the left next */
                 Vad = Vad->LeftChild;
             }
@@ -2453,6 +2456,11 @@ MiQueryMemoryBasicInformation(IN HANDLE ProcessHandle,
             {
                 /* Then this VAD is too low, keep searching on the right */
                 ASSERT(BaseVpn > Vad->EndingVpn);
+
+                /* Stop if there is no right child */
+                if (!Vad->LeftChild) break;

+
+                /* Search on the right next */
                 Vad = Vad->RightChild;
             }
         }


Releo, y me doy cuenta que algo no tiene sentido...el bug es casi evidente.

¿Eres capaz de encontrarlo?
Lee las líneas, aplica tus conocimientos de C (la pura intuición también vale) y/o sigue leyendo..





El Bug

 

Supongo que te habrás dado cuenta.
La primera y segunda línea tienen coherencia.

+                /* STOP si no hay un “child” a la izquierda *
+                if (!Vad->LeftChild) break; 
Si no hay un “child” a la izquierda, entonces  Vad->LeftChild tendrá un valor cero. Como “!0” es lo mismo que “1” se cumple que efectivamente no hay child a la izquierda y con el break “Paramos!”. El comentario y la linea siguiente parecen tener sentido.

Ahora bien, las dos siguientes no parecen tenerlas:
+                /* STOP si no hay un “child” a la derecha*/
+                if (!Vad->LeftChild) break;


Si estamos mirando el “child” de la derecha,¿porqué comprobamos el valor de LeftChild?Algo está equivocado ¿el comentario o el código?. Una de las principales reglas es hacer siempre caso al comentario
.
Pero aquí hay 3 evidencias que explican porqué se cometió el error y cual está equivocado:

1)Siempre hacer caso al comentario.

2)Comprobar el código.La siguiente línea es:

  Vad = Vad->RightChild;
En caso de que RightChild fuera 0, estaríamos asignando el valor 0 a “Vad”. Antes con el LeftChild intentamos evitar esta situación y por eso parábamos. Ahora también deberíamos hacerlo.

3)El Bug es un CopyPasta. Seguramente se copiaron las dos líneas del chequeo anterior pero solo se cambió la palabra “izquierda” con “derecha” en el comentario, y se olvidó cambiar el LeftChild por RightChild. Esto es muy frecuente.

Solo era necesario cambiar una única línea y todo resuelto. AQUÍ podéis ver el parche.

6 comentarios:

  1. Hola,
    Disuclpa mi osadía pero es Groucho Marx no Grouchoo Marx.
    Un saludo.

    ResponderEliminar
  2. Disculpado estás.Se me fue la mano con la "O"..
    Gracias :)

    ResponderEliminar
  3. A veces, los bugs son tan simples que pueden cargarse un ordenador... ;-)

    ResponderEliminar
  4. La verdad estoy muy entusiasmado con que este proyecto comience a andar de manera mas estable, mis conocimientos en computación no me ayudan a comprender mucho codigo y cosas parecidas, pero se que este sistema va correr como pan caliente. Mis felicitaciones, y ps siempre paso por aquí para ver el avance, que gracias a Uds. es muy considerable. Salu2.

    ResponderEliminar
  5. Es logico al no haber asignacion en los parametros especificados, se interrumpe la secuencia y queda el objeto en un punto muerto, el error es simple pero grave. Saludos.

    ResponderEliminar
  6. ¿Un bug en navidad?... ¡Qué valiente!... pensé que era broma y me he leido la news de un golpe... IMPRESIONANTE... he aprendido mucho ... de detalles para corregir bugs y de las magníficas persona que estan detrás de este proyecto...

    ResponderEliminar