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.

8 comments for “Det er et spørgsmål om stil

  1. Det er som du er inde på et spørgsmål om stil. Personligt kan jeg bedre lide den oprindelige stil. I øvrigt vil den sidste kodestump altid returnere fejlkoden 0 😉

    Jeg fristens til at omskrive PH. Der findes ikke dårlig kodestil, kun god, og den er dårlig.

    • Det er jeg ikke enig i. Det handler ikke om hvad man kan lide, eller ikke lide. Det handler om at kode på en måde så man laver så fejl som muligt og at konsekvensen af en evt. fejl bliver så lille så mulig.

      Hvis den person der har lavet det kode, havde valgt den stil jeg forslå ville de ikke havde efterladt 100’vis af milloner sårbare for angreb. Hvad der måske ikke er helt klar af eksemplet er at der er mange flere linjer kode i den funktion. Når man skal lave eksempler bliver det altid en mere simplificeret, men løsninger der ikke er trivielle kan en funktion/metode indeholde 100’vis af linjer kode, og så bliver det endnu vigtigere hvordan man koder.

      Jeg har masser af eksempler fra de gode gamle dage hvor man lave extensions til webservere ved at skrive native code, der blev kørt i samme process, at fejlbehæftet kode tog hele web serveren med ned. Hvis man brug en defensiv kodestil, ville det værst ofte være at en specifik funktion ikke virkede, ikke at hele sitet var nede.

      Så hvis du kan vise mig at man får færre fejl/bedre kvalitet ved at bruge en bestemt kode stil, så er det fint. Men at du bedre kan lide en stil frem for en anden kan jeg ikke rigtigt bruge til noget.

      • Nu er det ikke kodestilen som er skyld i fejlen, men at der indsat et ekstra goto statement. Som jeg skrev, har du selv en fejl i den kodestump du mener er af bedre kvalitet, hvilket er en smule komisk. Så hvad har du egentlig selv vist?

        • Nej, hvad jeg illustrere er at den stil der valgt at kode i, fører til at en fejl kortslutter alle efterfølgende check.

          Og med hensyn til mit kode eksempel så returnere den enten 0 som = ingen fejl, eller returnere den en fejlkode. Og skulle der undervejs værre tale om at resultatet ved et uheld bliver sat til 0 = ingen fejl, vil alle de efterfølgende check stadig blive udført, altså man fejler kun på den specifikke test, og ikke på alle efterfølgende test.

          Jeg siger ikke at kodestil jeg har lagt op til er den eneste brugbare, min erfaring viser at den stil du koder med kan havde store konsekvenser for kvaliteten/robustheden af koden. Derfor siger jeg også at det ikke handler om hvad man kan lide eller synes om, men hvad der rent logisk sker med din kode hvis du laver fejl, og hvilken stil der minimere risikoen for at lave fejl når du retter eller tilføjer kode.

          Som det er nævnt vil det at altid lave { } selv om det sprogligt ikke er nødvendigt, være med til at nedsætte risikoen for at sådan en fejl som er lavet i Apples kode have nogen konsekvens.

          • Ah ok, det er ikke helt klart af teksten. Jeg har dog svært ved at se forbedringen i forhold til den oprindelige kode når du ved en fejl sætter retur værdien til 0 i det sidste if statement. Det er muligt du får lavet checket på de efterfølgende test, men du smider information om eventuelle fejl opstået i de forgående.

  2. Enig!

    Har selv brændt fingrene på et if-statement uden {} som havde den korrekte indrykning ift hvad intentionen var. Jeg fandt ikke fejlen ved review, men først ved at debugge koden. Så siden da har jeg altid {}’er i mine IF’s og lignende i dag.

    I de 25 år hvor jeg har skrevet kode er jeg gået fra den minimalistiske kode stil til den mere explicite.

    Jeg tror det kan koges ned til erfaring.

    • Det er rigtigt.

      Meget af det med at definere en god kodestil, kommer fra erfaring, men også at man reflektere over de fejl man finder.

      Heldigvis er der i dag stærke IDE’er der kan fange og advare om mange af ups’erne. f.eks det med at komme til at lave en assignment i en if sætning. Hos os er der en klar regel, der siger at man ikke commit’er kode der indeholder warnings.

Skriv et svar til Claus Rathje Annuller svar

Din e-mailadresse vil ikke blive publiceret. Krævede felter er markeret med *