It would help a lot with bug-clarity if both the "record umask on
startup" and "add API to OS.File" changes were split into their own
bugs. The debate is really about the OS.File API.
The API question depends a lot on the use cases people foresee. Are
there any use cases identified for this API other than the download
manager?
OS.File is a fairly low-level API, so I tend to agree that it
shouldn't be too tailored to the download manager case specifically,
unless there's a lot of "demand" for that particular use case. A
generic "setPermisions" API seems to fit with the rest of the OS.File
API. "honorUmask" doesn't seem necessary at this point, unless there
are identified use cases for it not mentioned.
Gavin
On Fri, Apr 25, 2014 at 11:08 AM, Zack Weinberg <za...@panix.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> Bug 961080 asks for the new download manager to "support group and
> world-writable umasks for downloaded files". It is stalled on a design
> disagreement between me and the OS.File maintainers (well, it is also
> stalled because my development box corrupts its filesystem when I do 'hg
> pull -u', but that's not important right now) which I would like to
> raise for discussion here.
>
> In case you are unfamiliar, the 'umask' is how Unix-derived systems
> (including Mac) control whether newly created files are accessible to
> anyone beyond the creating user account. It's a process-global bit
> mask. When you create a file ...
>
> fd = open(path, O_RDWR|O_CREAT|O_EXCL, 0666);
>
> ... that third octal argument would normally cause the file to be
> created readable and writable by the current user, the current user's
> primary group, and everyone else (i.e. by everyone). But bits that are
> *set* in the umask are *cleared* from the permission bits passed to
> open(). So if the umask is 0027, the above open() will create a file
> with permission bits set to 0640 -- readable and writable by the current
> user, readable but not writable by the user's primary group, and
> inaccessible to anyone else.
>
> The new download manager is currently doing the moral equivalent of
>
> fd = open(path, O_RDWR|O_CREAT|O_EXCL, 0600);
> ^^^^
>
> which causes all downloaded files to be inaccessible to group and other,
> *regardless* of the umask setting. (The umask can only clear permission
> bits, it cannot set them.) The bug requests that we make it "honor the
> umask", i.e. go back to creating files as-if by open(,,0666). The module
> owners have decided that it is not safe to just change how we call
> open(); instead we need to reset the files' permissions manually *after*
> they are fully downloaded. This requires augmenting the OS.File API
> somehow, and this is what I added (quoting bug 961080 comment 15):
>
> | setPerms([path,] mode) - set the Unix file access permissions to a
> | specific numeric mode. Throws an exception if called on Windows.
> |
> | makeAccessibleByOthers([path,] exec) - Adjust file access permissions
> | to make the file accessible by others, honoring any relevant
> | system configuration about that. If 'exec' is true, make the
> | file executable, otherwise don't. On Unix, that's
> |
> | chmod(path, (exec ? 0777 : 0666) & ~umask);
> |
> | On Windows, currently does nothing. I suspect there is
> | something useful it could do on Windows, but I don't know
> | what it is.
>
> Both Paolo Amadini and David R. Teller objected to this API; their
> preferred API looks something like
>
> | unixSetPermissions([path,] mode [, options]) - set the Unix file
> | access permissions to 'mode & ~umask' by default; if 'options'
> | is '{ honorUmask: false }', then set them to exactly 'mode'
> | instead. Not present on Windows.
>
> I think their proposal is not a good API, and here's why. First, I
> don't think an operation named setPermissions() should honor the umask
> by default, because *most* set-permissions operations won't want to,
> and it's inconsistent with the C-level API (chmod()). "I have already
> created this file, now correct its permissions to what they would have
> been if the umask had been allowed to determine them" is an unusual
> scenario that should be easily greppable for; it deserves its own name.
>
> Second, if you are in a situation where you *do* want to honor the
> umask, then there are only two correct choices for the permissions to
> combine with the umask: 0666 if the file shouldn't be executable, 0777
> if it should (or if it's a directory). It is better to boil this down
> to a boolean ('exec') than to make all callers get it right.
>
> Third, there is probably something analogous that we should be doing
> on Windows; I am not qualified to know what it is, but I am 100%
> certain that this, too, should *not* be something that all callers
> have to get right, or even know about. Therefore I think that
> whatever new API is added should be defined in cross-platform terms
> (such as "make accessible by [appropriate] others").
>
> Against these (particularly the third) was the claim that this API is
> only useful to the download manager. I don't believe that. I suspect
> we have failure-to-honor-umask-or-Windows-analogue bugs in every place
> where we create a file outside the profile directory, and that
> therefore my proposed API is at least potentially relevant --
> everywhere we create files. And I believe it is correctly designed
> for the general case and not just the download manager.
>
> zw
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1
> Comment: Using GnuPG with Icedove - http://www.enigmail.net/
>
> iQIcBAEBCAAGBQJTWqSvAAoJEJH8wytnaapksg8P/jYu3oAAr5u8+devUCWN/Uos
> THhjyHq0DQKO5JzHWhAOVbQpUeD6TLkJdp5HPmCsodx3J416G0sG9D+4DqJ5tpQU
> tGahDaPwJcnjEaq/KhzhxEq6s6E7z0n01Pu5YxgC84MMTWsvIcuVEsoYZBKDNo6y
> Y34qASTB57OhjO164aqNBLblm9nZPCPdMqLg8TeFA2EeO6pmy4enaxj+XEcED7yr
> CMziKu0Ild2gVXDzUNaiTALNlkItDsP+aFi1EV3PLoX2EgJObzIokGQ99uySX15q
> SNUG1lF5X4k1USICnkfqDTTi2Jr0kAjGJp7bZs+v+fNs8i18w1XK0ERU+7G2t8jF
> uF91ame9QunfJAr0kO+ezEcMKB6fGQOkpk0TvaBNNQIaLeTVRRjTO19WQfQkej8y
> ddmdy5Pi+LjoWUqL9vjRYp6Zk/bdi4ovC1V8tFDVEDg9l5uzsdnnctZI7GajCAqw
> mjZdkDwvNA+8DaAzAWX0CtC9ivmXW7Wq+U0c42Gf5LgqnorO544xbfXu0KFzG9l4
> nYv7Jfc2p/TU2EdN8xxPk7v4UaG/b6HF+N4oLDUuPsv2JNuKFcv754OzyEMDDJ8n
> bU3OmZMoymRuRqSoPiDFt96vQSLl3ZxmMvPVB5mCIqMAFkrC8xm94l7nijamMAa8
> UPtBM/LBuwnorGQfn9TF
> =DFbE
> -----END PGP SIGNATURE-----
> _______________________________________________
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
