A few days ago, jsing fixed a bug in libssl. It’s not the end of the world, but it’s a bug. Here’s the diff.

 int
 ssl3_final_finish_mac(SSL *s, const char *sender, int len, unsigned char *p)
 {
-	int ret;
-	ret = ssl3_handshake_mac(s, NID_md5, sender, len, p);
-	p += ret;
-	ret += ssl3_handshake_mac(s, NID_sha1, sender, len, p);
-	return (ret);
+	int ret_md5, ret_sha1;
+
+	ret_md5 = ssl3_handshake_mac(s, NID_md5, sender, len, p);
+	if (ret_md5 == 0)
+		return 0;
+	p += ret_md5;
+	ret_sha1 = ssl3_handshake_mac(s, NID_sha1, sender, len, p);
+	if (ret_sha1 == 0)
+		return 0;
+	return (ret_md5 + ret_sha1);
 }
 
 static int

Two variables are introduced, one for each function called, and the local variable for this function’s return value is removed.

Now look at the OpenSSL fix for the same bug.

 int ssl3_final_finish_mac(SSL *s, 
 	     const char *sender, int len, unsigned char *p)
 	{
-	int ret;
+	int ret, sha1len;
 	ret=ssl3_handshake_mac(s,NID_md5,sender,len,p);
+	if(ret == 0)
+		return 0;
+
 	p+=ret;
-	ret+=ssl3_handshake_mac(s,NID_sha1,sender,len,p);
+
+	sha1len=ssl3_handshake_mac(s,NID_sha1,sender,len,p);
+	if(sha1len == 0)
+		return 0;
+
+	ret+=sha1len;
 	return(ret);
 	}
 static int ssl3_handshake_mac(SSL *s, int md_nid,

A new variable is introduced for one function, but the existing variable for this function’s return value continues to be used to store a called function’s return.

Why is this bad? As alluded to earlier, improper return checking bugs are rampant in TLS libraries. Apple’s #gotofail bug and the following week’s GnuTLS bypass were both caused by setting a return code too soon, and then bubbling it up. Here we have the identical bug (diminished in severity), and the opportunity to correct a poor practice by eliminating semantic overloading of variables. Why not take it?