Ensure umask is unset when extracting archive

[natalieparellano]

Thanks @yaelharel for prodding me to look into this more closely

#203 added logic to update file permissions in case the system umask was applied.
#246 unset the umask before the extract to avoid having to do this extra step (thus saving time), however it made the function not thread-safe as discovered in #719.

This PR ensures that

• the umask is always unset when running the extract
• the system umask is applied to directories
• extracts running in parallel will not contaminate each other

@sclevine it looks like you have some of the history here, it would be great if you could take a look!

cc @aemengo @jabrown85

[jabrown85]

Can we, instead of trying to make Extract thread-safe handle umask, put the umask reset logic in restorer?

This is the bit that causes the problem, if I'm understanding this correctly.

Could we make (r *Restorer) Restore(cache Cache) error have the init like we used to have?

func init() {
	systemUmask = setUmask(0)
	_ = setUmask(systemUmask)
}

func (r *Restorer) Restore(cache Cache) error {
	umask = setUmask(0)      
        defer setUmask(systemUmask)
        ... some time later ...
        go layers.Extract(rc, "", umask) // add umask as param to Extract
}

We don't expect Restorer to be used concurrently and we know Extract is used concurrently so it makes sense to me to move the unsafe bits up a level.

Or, if we really don't want to pass umask from restorer down to archive, we could have archive.Extract only execute umask(0) if /proc/self/status umask is not already zero and always return the previous umask if it had to set it so the caller can do something like this so Extract is safe for reentry.

var firstUmask

go func(){
   umask, err := layers.Extract(rc, "")
   if umask != -1 {
    firstUmask = umask
  }
}

// after all are done
setUmask(prevUmask)

[natalieparellano]

put the umask reset logic in restorer

The latest incarnation of this PR does this. archive.Extract doesn't do anything except verify that the umask is already 0. Maybe we could take the verification out, and add a docs comment, but that would make the function buggy for anyone who didn't read carefully, which seems bad.

Could we make (r *Restorer) Restore(cache Cache) error have the init like we used to have?

What would the variable set in the init be used for?

we could have archive.Extract only execute umask(0) if /proc/self/status umask is not already zero and always return the previous umask if it had to set it

This is interesting. I had some variation of this before, but I was erroring out if the umask was not already 0 (and trying to "set it back" within archive.Extract, which is unsafe). Let me give it a try.

[natalieparellano]

@jabrown85 one bit that I think makes what you proposed hard is that the original umask is also used within archive.Extract, here:

lifecycle/archive/extract.go

Line 57 in 8c2edb2

if err := os.MkdirAll(dirPath, applyUmask(os.ModePerm, umask)); err != nil {

[natalieparellano]

After conversation with @jabrown85, we decided:

• Getting the system umask in an init function is potentially too early (a program could alter it later on)
• We don't need to "check the current umask by unsetting it" inside archive.Extract if we document that the umask must be unset prior to calling the function
• We don't entirely trust our homegrown GetUmask()

The latest commits reflect this thinking. It's a bit gross to change the signature of archive.Extract and layers.Extract, but it's safer this way (we think).

[aemengo]

Very elegant!

[natalieparellano]

The arm worker... 😭

[yaelharel]

Since we're calling this function only in one use case, maybe we can put it inside the for loop. I'm not sure what will be clearer for the user.

[natalieparellano]

Aye, I called this function in two places before and forgot to change it back. I would update but I'm having problems with my IDE... let's just leave it out of laziness.

[yaelharel]

Added a few small comments but I don't have strong opinion on making those changes.

[codecov]

Codecov Report

Merging #727 (c8addc4) into release/0.12.0 (7e50a62) will increase coverage by 0.10%.
The diff coverage is 37.50%.

❗ Current head c8addc4 differs from pull request most recent head 9f3d758. Consider uploading reports for the commit 9f3d758 to get more accurate results

@@                Coverage Diff                 @@
##           release/0.12.0     #727      +/-   ##
==================================================
+ Coverage           64.65%   64.74%   +0.10%     
==================================================
  Files                  52       52              
  Lines                3668     3669       +1     
==================================================
+ Hits                 2371     2375       +4     
+ Misses               1048     1045       -3     
  Partials              249      249              

Flag	Coverage Δ
os_windows	64.74% <37.50%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.