[release/6.0] Fix setting timestamp on Windows on readonly files #62922
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsBackport of #62638 Customer ImpactAddresses a DTS bug that has been reported internally. TestingA regression test has been added and it's passing. RiskLow, as the fix is about using less restrictive access rights when opening a file handle to modify the file attributes ( cc @ericstj
|
|
Hmm, out of an abundance of caution, given this behavior has existed for many years, including on .NET Framework, and some might believe it is expected behavior, would it make sense to add an app context flag, just for the backport? I don't have any concerns about 7.0, but in servicing we don't expect customers to change their code. @marklio thoughts? Note, this brings behavior in line with Linux; any issue would be in some code written for Windows, possibly long ago. |
danmoseley
changed the base branch from
backport/pr-62819-to-release/6.0
to
release/6.0
# Conflicts: # src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs
danmoseley
force-pushed
the
backport62638
branch
from
f53d176
to
41ee8fe
Compare
|
@adamsitnik what are your thoughts about including an app context switch (default to the new behavior)? If there is a customer broken by this, they would still be able to receive the other fixes in the 6.0.2 patch. There is no need for a switch in main/7.0 as that is a voluntary update. Worst case, nobody uses it (I suspect nobody will) |
leecow
added
Servicing-approved
|
We don't typically quirk removal of an exception like this. I chatted with @danmoseley and he mentioned he's OK with this as-is. |
|
@dotnet/dnceng there are CI legs here that have been running for 5 days. Should there be some timeout applying here? I"ll rerun. |
Should yes, usually anything over a day is a missing call to a web hook or other API. I'll poke around a bit to see if I can find anything more nefarious. |
|
Just saw this back from vacation. Since I was summoned, I'll weigh in even though I think stakeholders have reached consensus already. It's going to be pretty expensive to try to draw any conclusions based on existing usage (meaningful exception handling comprehension in static analysis tools is very tricky), so I'm not sure it's worth it. On .NET Framework, we would definitely not change this default behavior in servicing. If there was a request with business justification, we'd quirk it for a new release and allow folks to opt-in in a servicing update if we could appropriately manage the risk of the change (we probably could). For the Core model, this seems analogous to changing the behavior in a new release. That said, I don't think the risk is very high that folks are using these calls as last line of defense "is readonly" checks, so I think the value of consistency between OS platforms, and moving to behavior we think is correct likely wins out here. I'd definitely document it as a breaking change though. |
|
Thanks @marklio. Note this is servicing, it's already merged into 7.0. I'm not sure from your comment about major version whether you had noted that. Good reminder to doc it though |
ghost
locked as resolved and limited conversation to collaborators
Backport of #62638
Fix #62602
Customer Impact
Addresses a DTS bug that has been reported internally. Attempting to modify the timestamp of a read-only file on Windows with an API like
File.SetXXTimeXXthrows UnauthorizedAccessException.This is not a regression. The exception is thrown on.NET Framework as well. It’s clearly a bug (we’re passing the wrong flag) and it has always succeeded on Linux. We already fixed it for 7.0. In theory a customer could be depending on the current behavior, but our assumption is that this is unlikely.
Testing
A regression test has been added and it's passing.
Risk
Low, as the fix is about using less restrictive access rights when opening a file handle to modify the file attributes (
FILE_WRITE_ATTRIBUTESinsteadGENERIC_WRITEwhich combinesFILE_WRITE_ATTRIBUTESand many other flags) .cc @ericstj