Det er et spørgsmål om stil

Det er et spørgsmål om stil Advarsel. Dette handler om Apples goto fejl, men ikke direkte om sikkerhed, men meget teknisk om kodestandard og stil. Hvis du ikke har hørt om den kan du se mere her https://www.imperialviolet.org/2014/02/22/applebug.html Apples goto fail fejl er et fantastisk eksempel på hvor store konsekvenser en lille fejl kan have. Der er der allerede skrevet rigeligt om. I stedet vil jeg skrive lidt om min kæphest gennem de over tyve år jeg har lavet kode. At den kodestil man bruger kan have en stor betydning for kvaliteten af koden. Både læsevenlighed, intention og fejlrate kan påvirkes markant. Nogen gange kan der være store og næsten religiøse diskussioner om hvilken metode/stil der er bedst, men i langt de fleste tilfælde kan en stil kvalificeres. Jeg har af flere omgange været ansvarlig for hvilken kodestil der brugt til udvikling. Det er noget jeg har dikteret hårdt, og hvis nogen har været uenig har de skulle komme med noget målbart for at få det ændret. Men lad os se på koden fra Apple, som er open source og vi derfor kan se stillen af.

static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
	OSStatus        err;
	...

	if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
		goto fail;
	if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
		goto fail;
		goto fail;
	if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
		goto fail;
	...

fail:
	SSLFreeBuffer(&signedHashes);
	SSLFreeBuffer(&hashCtx);
	return err;
}

Problemet er den ekstra goto fail linje. En type årsag til dette problem kunne være at man havde fjernet en if sætning fordi der var et check som ikke længere var nødvendigt, men man har glemt at få goto sætningen med. Denne kode kunne være et klart eksempel fra min side på en kode stil der er svær at læse, debugge og potentielt giver masser af fejl. Den kan omskrives til følgende:

fejlkode funktionskald(parameter)
{
status	err;
	….

if((err = test1) != 0)
	goto fail;
if((err = test2) != 0)
	goto fail;
	goto fail;
…..

if((err= testX) != 0)
	goto fail;

	….

fail:
	cleanup
	return err;
}

Normalt ønsker man aldrig at bruge goto, men her er man i det mindste indenfor scope af en enkelt funktion. Problemet med goto er det er meget svært at overskue flow i funktionen. Kode stilen med at undlade tuborgklammer for enkelt linje udtryk er også en dårlig ide, og i dette tilfælde, måske den væsentligste årsag. Det er dog ikke sikkert det havde gjort nogen forskel, men måske havde det været nemmere at opdage.

if(err = test1) != 0)
{
	goto fail;
}

if(err=test2) != 0)
{
	goto fail;
	goto fail;
}

Ville have reddet det hvis det var fordi man var kommet til at indsætte en ekstra goto, men hvis tesen var at man havde fjernet en if sætning, så ville det ikke. så ville det have set sådan ud, og fejlen havde stadig været der. Det springer dog mere i øjnene.

if(err = test1) != 0)
{
	goto fail;
}

if(err=test2) != 0)
{
	goto fail;
}

{
	goto fail;
}

Hvad der reelt er galt er hele den grundlæggende metode der bruges i flowet i funktionen. Hvis vi bruge noget best practice her, så handler det om følgende.

• En funktion bør have en indgang og udgang.
• Et resultat bør default sættes i starten til den værdi der giver mindst skade hvis den er forkert.
• Der bør ikke være assignments i if sætninger (specielt i C sprog) da man ofte kan ende med at lave fejl så man enten lavet et boolsk udtryk, eller laver en assignment ét stedet for et boolsk udtryk.
• Der bør altid bruges tuborgklammer, også i enkelt linjer. Det øger læsevenligheden, og minimere antallet af scope fejl.
• Hvis testflows er afhængige af hinanden bør der bruges konstruktioner der viser det, f.eks. if-then-else

Hvis vi bruger denne kodestil hvordan ville koden så komme til at se ud? Den kunne se sådan ud:

fejlkode funktionskald(parameter)
{
status	err = default_fejlkode;
	….

	err = test1;
if(err == 0) // vi må antage at 0 er uden fejl.
{
	err = test2;
}
if(err == 0)
{
	err=testX;
}
	if(err == 0)
	{
		….
	}

	cleanup
	return err;
}

Her er flowet hvor vi antager det går galt, og det ikke er et lovligt certifikat. Så længe testen vi laver giver et gyldig certifikat, laver vi ekstra test. Er certifikatet ikke gyldigt falde vi gennem alle tests og rydder op og returnere en fejl. Hvis alle testene lykkes vil fejlkoden være ok, og certifikatet gyldig. Det er naturligvis stadig mulig at ændre i denne kode og få fejl, men den er mere robust. F.eks. hvis vi laver en fejl om i Apples tilfælde, vi alle test stadig blive udført. Man er som udvikler, chef og forretning nødt til at forholde sig aktivt til den kodestil deres bruges når der kodes. Det kan have en stor betydning for kvaliteten af koden, og konsekvenserne når der laves en fejl. Heldigvis er det meget nemt med IDE’er idag at lave og dele en kodestil. Nogle IDE’er kan sågar sættes til at håndhæve en stil når commit’es til et source repository. Så kom bare i gang. Få talt om hvordan i koder, og hvad jeres kodestil skal være. Det er en del af at være en god håndværker.