[tool] when writing to openssl as a part of macOS/iOS code-signing, flush the stdin stream before closing it #150120
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
c870693
to
338c6b8
Compare
This comment was marked as resolved.
This comment was marked as resolved.
| stdin: opensslProcess.stdin, | ||
| line: signingCertificateStdout, | ||
| onError: (Object? error, _) { | ||
| throwToolExit('Failed to write to openssl process. Error: $error'); |
There was a problem hiding this comment.
Is this actionable by the user? We usually throw a tool exit if either the user provided invalid inputs or the state of their machine is in some bad state. This might legitimately be a bug in tool assumptions though, right?
Also, I wonder if the fact that we were synchronously closing this after we call .write() (but maybe before the write happened) is why this was crashing in the first place. If that is the case, this change should solve that.
There was a problem hiding this comment.
Is this actionable by the user? We usually throw a tool exit if either the user provided invalid inputs or the state of their machine is in some bad state. This might legitimately be a bug in tool assumptions though, right?
Great catch. I think I went on autopilot here. I've since replaced this with an Exception (code).
Also, I wonder if the fact that we were synchronously closing this after we call .write() (but maybe before the write happened) is why this was crashing in the first place. If that is the case, this change should solve that.
This is certainly possible. As you pointed out, Process.writeToStdinGuarded should protect us from this going forward.
| onError: (Object? error, _) { | ||
| throw Exception('Unexpected error when writing to openssl: $error'); | ||
| }, |
There was a problem hiding this comment.
If this ends up becoming a pattern (it very much probably will), we may want to consider creating like a writeToStdinUnsafe method that accepts something like a message or taskDescription. This new method would then throw a custom exception type from that.
Fixes #100584. Might help #137184.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.