fix the calling convention on the bunch of stuff #2877
Conversation
| . TLetD fail BX (TCon Ty.failureRef 0 [stack1, stack2]) | ||
| [ (0, ([BX, BX, BX],) | ||
| . TAbss [stack1, stack2, stack3] | ||
| . TLetD fail BX (TCon Ty.failureRef 0 [stack1, stack2, stack3]) |
There was a problem hiding this comment.
failure has 3 constructor arguments, not two
| outIoFailBox :: forall v. Var v => v -> v -> v -> v -> ANormal v | ||
| outIoFailBox stack1 stack2 fail result = | ||
| outIoFailBox :: forall v. Var v => v -> v -> v -> v -> v -> ANormal v | ||
| outIoFailBox stack1 stack2 stack3 fail result = |
There was a problem hiding this comment.
failure has 3 constructor arguments, not 2
| $ outIoFailBox stack1 stack2 fail result | ||
| where (unit, stack1, stack2, fail, result) = fresh5 | ||
| $ outIoFailBox stack1 stack2 stack3 fail result | ||
| where (unit, stack1, stack2, stack3, fail, result) = fresh6 |
There was a problem hiding this comment.
failure has 3 constructor arguments
|
oops, I mislabeled the branch, this actually fixes #2834 |
| flatten (Right (Left e)) = Left (Failure Ty.tlsFailureRef (Util.Text.pack (show e)) unitValue) | ||
| flatten (Right (Right (Left e))) = Left e | ||
| flatten (Right (Right (Right a))) = Right a | ||
|
|
There was a problem hiding this comment.
this new version operates on a a -> IO (Either Failure b) instead of a -> IO b
| Left l -> Left l | ||
| asCert :: PEM -> Either String X.SignedCertificate | ||
| asCert pem = X.decodeSignedCertificate $ pemContent pem | ||
| in | ||
| declareForeign Tracked "Tls.decodeCert.impl.v3" boxToEFBox . mkForeignTls $ | ||
| declareForeign Tracked "Tls.decodeCert.impl.v3" boxToEFBox . mkForeignTlsE $ |
There was a problem hiding this comment.
call our new version which expects us to return an Either Failure a instead of just a when we were just using the version that ignored the fact that we were returning an Either, all values looked like success unless they threw exceptions. in the case of bad data, the Left was misinterpreted as success
| test> this_should_not_work=match (decodeCert.impl (toUtf8 not_a_cert) with | ||
| Left _ -> [Ok "failed"] | ||
| Right _ -> [Fail "um, that was a schmificate"] | ||
|
|
There was a problem hiding this comment.
this catches the previous error where failures were ignored because of the broken calling convention
| @@ -4,42 +4,26 @@ | |||
| -- generated with: | |||
| -- openssl req -newkey rsa:2048 -subj '/CN=test.unison.cloud/O=Unison/C=US' -nodes -keyout key.pem -x509 -days 3650 -out cert.pem | |||
|
|
|||
| join strs = List.foldLeft (a -> b -> b ++ a ++ "\n") "" strs | |||
There was a problem hiding this comment.
this was broken, it should have been b ++ "\n" ++ a and this is why adding a newer cert didn't work and exposed the big in decodeCert
|
Amazing, thanks @stew. I will hold off on ✅ because I don't know enough about the runtime. |
|
I think that this shows that a lot of our IO stuff is under tested, especially in the Failure cases |
I will continue on the path of fixing very complex bugs that I myself introduced in order to make myself look smarter ;) |
Things that were returning failures were not getting all three constructor arguments from the stack.
We were never possibly handling faliures to
decodeCertcorrectly before. This also adds a test for certs that should fail to parseThis reinstates the test that was failing that we commented out as a fix for the recent problems with TLS handshakes