Opened 6 years ago

Closed 5 years ago

#5228 defect closed fixed (fixed)

twisted.test.test_paths.FilePathTestCase.test_getPermissions_Windows creates undeleteable file

Reported by: Jean-Paul Calderone Owned by: Jean-Paul Calderone
Priority: high Milestone:
Component: core Keywords: easy
Cc: Branch:
Author:

Description

This test method sets the mode of sub1 such that it cannot be deleted in the usual way:

[Error 5] Access is denied: '_trial_temp
twisted.test.test_paths
FilePathTestCase
test_getPermissions_Windows
bvk9lu
temp
sub1'

The test should ensure that regardless of the test outcome, this file ends up deletable, or it should delete it itself.

Attachments (5)

changed_file_permission_to_777.patch (478 bytes) - added by Indradhanush Gupta 5 years ago.
Changed the file permission to 777 in lieu of the sanity check.
create_deleteable_files.patch (807 bytes) - added by Max Haarhaus 5 years ago.
Change the final chmod command in test_getPermissions_Windows to allow sub1 to be deleted, while maintaining the user permissions = group permissions = other permissions test.
change_permissions_after_test.patch (581 bytes) - added by Max Haarhaus 5 years ago.
Change the file permissions during cleanup to allow for deletion.
change_permissions_after_test2.patch (918 bytes) - added by Max Haarhaus 5 years ago.
Include the news file.
5228.patch (715 bytes) - added by Max Haarhaus 5 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 6 years ago by gdeng

Owner: set to gdeng

Changed 5 years ago by Indradhanush Gupta

Changed the file permission to 777 in lieu of the sanity check.

comment:2 Changed 5 years ago by Indradhanush Gupta

Keywords: review added
Owner: changed from gdeng to Jean-Paul Calderone

comment:3 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Indradhanush Gupta

Thanks for working on this dhanush. I'm not sure why this is assigned to me, though. The general procedure is to assign tickets for review to noone (the empty entry at the top of the "assign to" dropdown). I filed this ticket, but that doesn't mean I have to review it.

However, since I'm here and the patch is very short, I guess I will.

My only comment on the patch is that it doesn't seem to address the issue. It changes an assertion being made in the test method. This doesn't seem correct to me, the assertion being made before was correct, so the new assertion after this patch is probably incorrect. The tests only runs on Windows so I can't easily run it, but I suppose that if it were run, it would fail.

Beyond that, the problem described in the ticket description still exists. The file is still created with permissions that make it undeleteable.

comment:4 in reply to:  3 Changed 5 years ago by Indradhanush Gupta

Thanks for your feedback. I will stick to the general procedure for assigning tickets to review. Regarding the patch, as the issue still stands, I'll try to take a plunge in more detail into the code and try to fix this up in a future patch.

Changed 5 years ago by Max Haarhaus

Change the final chmod command in test_getPermissions_Windows to allow sub1 to be deleted, while maintaining the user permissions = group permissions = other permissions test.

comment:5 Changed 5 years ago by Max Haarhaus

Keywords: review added
Owner: Indradhanush Gupta deleted

Change the final chmod command in test_getPermissions_Windows to allow sub1 to be deleted, while maintaining the user permissions = group permissions = other permissions test.

comment:6 Changed 5 years ago by Tom Prince

Keywords: review removed
Owner: set to Max Haarhaus

The change looks good, but it could probably be better. If the assertion triggers the second time through the loop, it will still leave an undeletable file.

TestCase.addCleanup could be used to avoid this. It would also be useful to add a comment as to why it needs to be done, so future maintainers don't break it again.

comment:7 Changed 5 years ago by Max Haarhaus

Maybe I'm looking at it wrong, but isn't the final chmod (to 511, making it undeleteable) occur outside of the loop? The mode is changed to 777 and tested then 555 and tested inside the loop, then with my change, 711 and tested outside.

Regardless, I'll look into doing it with addCleanup instead.

Changed 5 years ago by Max Haarhaus

Change the file permissions during cleanup to allow for deletion.

comment:8 Changed 5 years ago by Max Haarhaus

Keywords: review added
Owner: Max Haarhaus deleted

comment:9 Changed 5 years ago by Tom Prince

Maybe I'm looking at it wrong, but isn't the final chmod (to 511, making it undeleteable) occur outside of the loop? The mode is changed to 777 and tested then 555 and tested inside the loop, then with my change, 711 and tested outside.

If none of the assertions fail, then that is the case. But if one the assertions fails, then the code will interrupted, and the file could be left in an undeletable state.

comment:10 in reply to:  9 Changed 5 years ago by Max Haarhaus

Replying to tom.prince:

Maybe I'm looking at it wrong, but isn't the final chmod (to 511, making it undeleteable) occur outside of the loop? The mode is changed to 777 and tested then 555 and tested inside the loop, then with my change, 711 and tested outside.

If none of the assertions fail, then that is the case. But if one the assertions fails, then the code will interrupted, and the file could be left in an undeletable state.

Okay, I understand. The new patch uses addCleanup, so it should avoid that issue.

Changed 5 years ago by Max Haarhaus

Include the news file.

comment:11 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: set to Max Haarhaus

The problem with doing the cleanup registration at the end of the function is that if one of the assertion fails earlier one, or the test is interrupted, it will never be reached. So cleanup will still not happen if an assertion fails early on.

comment:12 Changed 5 years ago by Itamar Turner-Trauring

Oh, and the change is minor enough the news file could just be a .misc with no content.

Changed 5 years ago by Max Haarhaus

Attachment: 5228.patch added

comment:13 in reply to:  11 Changed 5 years ago by Max Haarhaus

Keywords: review added
Owner: Max Haarhaus deleted

Replying to itamar:

The problem with doing the cleanup registration at the end of the function is that if one of the assertion fails earlier one, or the test is interrupted, it will never be reached. So cleanup will still not happen if an assertion fails early on.

Yeah, silly mistake. Fixed that and changed the news file.

comment:14 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jean-Paul Calderone

Thanks! This looks like the right fix. I'll apply it to trunk.

comment:15 Changed 5 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [38294]) Clean up test state in the FilePath tests on Windows so that _trial_temp can be deleted

Author: mhaarhaus Reviewer: tom.prince, itamar, exarkun Fixes: #5228

Previously a FilePath test left some files in _trial_temp without permissions necessary to delete them. This made clean up _trial_temp a pain. The test now makes sure to restore permissions on the dummy files after the test is over.

Note: See TracTickets for help on using tickets.