-
Notifications
You must be signed in to change notification settings - Fork 26.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[tool] when writing to openssl as a part of macOS/iOS code-signing, flush the stdin stream before closing it #150120
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
packages/flutter_tools/test/general.shard/ios/code_signing_test.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few nits, but LGTM
Fixes #100584. Might help #137184.
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.