Closed Bug 1168231 Opened 9 years ago Closed 9 years ago

Normalize file mode in jars

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

When building with a different umask, jar creation ends up using different file modes.
Assignee: nobody → mh+mozilla
Attachment #8610489 - Flags: review?(gps)
Comment on attachment 8610489 [details] [diff] [review]
Normalize file mode in jars

Review of attachment 8610489 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozpack/files.py
@@ +219,5 @@
> +        # - apply a standard umask
> +        # - keep file type (e.g. S_IFREG)
> +        file_type = stat.S_IFMT(mode)
> +        mode = mode & stat.S_IRWXU;
> +        mode = mode | (mode >> 3) | (mode >> 6)

Bit shifting is correct, but too clever for my liking. Please use S_IRWXG and S_IRWXO.

@@ +220,5 @@
> +        # - keep file type (e.g. S_IFREG)
> +        file_type = stat.S_IFMT(mode)
> +        mode = mode & stat.S_IRWXU;
> +        mode = mode | (mode >> 3) | (mode >> 6)
> +        return file_type | (mode & 0755);

It feels a bit weird to mix and match constants with the octal magic number. But it's file modes, so hopefully everyone groks that you are stripping group and other writable bits here.
Attachment #8610489 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #2)
> Comment on attachment 8610489 [details] [diff] [review]
> Normalize file mode in jars
> 
> Review of attachment 8610489 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozpack/files.py
> @@ +219,5 @@
> > +        # - apply a standard umask
> > +        # - keep file type (e.g. S_IFREG)
> > +        file_type = stat.S_IFMT(mode)
> > +        mode = mode & stat.S_IRWXU;
> > +        mode = mode | (mode >> 3) | (mode >> 6)
> 
> Bit shifting is correct, but too clever for my liking. Please use S_IRWXG
> and S_IRWXO.

S_IRWXG and S_IRWXO are resp. 070 and 007. They're not going to help doing what this line does, which is replicate the same mode bits from users to group and others.

> @@ +220,5 @@
> > +        # - keep file type (e.g. S_IFREG)
> > +        file_type = stat.S_IFMT(mode)
> > +        mode = mode & stat.S_IRWXU;
> > +        mode = mode | (mode >> 3) | (mode >> 6)
> > +        return file_type | (mode & 0755);
> 
> It feels a bit weird to mix and match constants with the octal magic number.
> But it's file modes, so hopefully everyone groks that you are stripping
> group and other writable bits here.

I thought stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP | stat.S_IROTH | stat.S_IXOTH would be overkill. In fact, I find it less readable than 0755... in fact, come to think of it, I'd rather replace stat.S_IRWXU with 0700. Thoughts?
Or... that could be rewritten as:
 ret = file_type
 if (mode & stat.S_IRUSR)
     ret |= stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH
 if (mode & stat.S_IWUSR)
     ret |= stat.S_IWUSR
 if (mode & stat.S_IXUSR)
      ret |= stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH
 return ret
I like comment #4 because it constructs the final mode from scratch rather than creating something that's undone by a bit mask in the very last step. It's also clear that read and execute bits are global but write only applies to user. I would also accept this code with octal literals in it. e.g.

ret = file_type
# Read and execute privileges become global
if mode & 0400:
  ret |= 0444
if mode & 0100:
  ret |= 0111
# Write privileges only apply to owner
if mode & 0200:
  ret |= 0200

return ret
https://hg.mozilla.org/mozilla-central/rev/55b6a6d3e2be
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: