Skip to content

[flutter_tools] check for permission issues when copying file#63540

Merged
fluttergithubbot merged 15 commits intoflutter:masterfrom
jonahwilliams:permission_issue
Aug 21, 2020
Merged

[flutter_tools] check for permission issues when copying file#63540
fluttergithubbot merged 15 commits intoflutter:masterfrom
jonahwilliams:permission_issue

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Aug 12, 2020

Description

We're seeing a large number of file system crashes on the latest stable. To determine whether it is an issue reading or copying, first attempt to access some bytes of the template file to force any permissions errors to be shaken out.

If this is the case, exit with an informative error instead of crashing.

#63528

@flutter-dashboard flutter-dashboard bot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Aug 12, 2020
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might have given bad advice that it's easy to make ErrorHandlingFileSystem handle what _validateReadPermissions is doing. But as written, because openSync() gets forwarded to the delegate file system, I don't think this change will give the tool exit. It will still give the crash.

/// If this fails with a certain error code, the [ErrorHandlingFileSystem] will
/// trigger a tool exit with a better message.
void _validateReadPermissions(File file) {
final RandomAccessFile randomAccessFile = file.openSync();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like ErrorHandlingFile overrides openSync().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, yeah I did not run my integration test with this change after switching to the error handling file system. I could modify this so that it skips the random access file.

final File file = fs.file('file');

const String expectedMessage = 'The flutter tool cannot access the file';
expect(() async => await file.writeAsBytes(<int>[0]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is good. A test that you get a tool exit for the operations that _validateReadPermissions is also needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return fileCount;
}

/// Attempt to the length from the file to ensure that read permissions are correct.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Attempt to the length from the file to ensure that read permissions are correct.
/// Attempt to find the length from the file to ensure that read permissions are correct.

@jonahwilliams
Copy link
Contributor Author

( I haven't actually verified that reading the file length hits the permission bit check on windows)

@jonahwilliams jonahwilliams requested a review from zanderso August 20, 2020 23:36
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm w/ nit

setupWriteMocks(
mockFileSystem: mockFileSystem,
fs: fs,
errorCode: 5,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include this above with the other constants starting on line 83.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants