SetPermissionsOfLink sets permissions on link target on UNIX systems

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: jaas, Assigned: jaas)

Tracking

Trunk
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

785 bytes, patch
smichaud
: review+
Details | Diff | Splinter Review
Assignee

Description

11 years ago
In nsLocalFileUnix.cpp, SetPermissionsOfLink is implemented by just calling SetPermissions, which always acts on link targets via chmod. It should act on the link itself via lchmod.
Assignee

Comment 1

11 years ago
Posted patch fix v1.0 (obsolete) — Splinter Review
Assignee

Updated

11 years ago
Attachment #340762 - Flags: review?(smichaud)
Assignee

Updated

11 years ago
Summary: SetPermissionsOfLink is wrong on UNIX systems → SetPermissionsOfLink sets permissions on link target on UNIX systems
Assignee

Comment 2

11 years ago
We have to check on the availability of lchmod, it doesn't exist on Mac OS X before 10.5 (not relevant here, just saying) and I think there are at least some recent versions of Solaris that don't support it.

I think BSD and Linux versions that we'd be concerned with have lchmod, and Solaris ignores the permissions of symlinks (basic googling), so perhaps we can just ifdef and return NS_OK for Solaris.
Comment on attachment 340762 [details] [diff] [review]
fix v1.0

Josh, you (probably) shouldn't use lchmod().

When I compiled a simple test program on my Linux box (Ubuntu 7.10)
that used lchmod(), I got the following warning:

warning: warning: lchmod is not implemented and will always fail
Attachment #340762 - Flags: review?(smichaud) → review-
Assignee

Comment 4

11 years ago
Whoa, I didn't get that at all. I'll figure out the story there. We should at least be doing nothing instead of incorrectly mucking with the link target.

Comment 5

11 years ago
Are there file systems where symlinks *can* have permissions? I've never heard of such a thing.

AFAICT we'd be better off throwing a NOT_IMPLEMENTED exception in all cases.
i agree.  the current implementation is very wrong.
Assignee

Comment 7

11 years ago
Posted patch fix v1.1Splinter Review
Fine with me - I'm only really interested in not targeting the wrong file. We can sort out implementing this in another bug if people want.
Attachment #340762 - Attachment is obsolete: true
Attachment #341048 - Flags: review?(smichaud)
Attachment #341048 - Flags: review?(smichaud) → review+
Assignee

Updated

11 years ago
Attachment #341048 - Flags: superreview?(doug.turner)
Comment on attachment 341048 [details] [diff] [review]
fix v1.1

this is what we discussed.  Please post something to the newsgroup regarding this change.
Attachment #341048 - Flags: superreview?(doug.turner) → superreview+
Assignee

Comment 9

11 years ago
landed on trunk
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.