pkg/docker: lock auth.json before reading/writing #1506
Conversation
|
Where is the lock for writing? |
Each auth.json file is used as a lock. This way, we don't have to worry about finding a path for a lockfile for (root)less on Mac, Win, and Linux. |
|
Doesn't seem to work yet as I want to though. |
|
Not what I meant. I would think there is a code path when writing the auth.json file that should take the lock, but this PR only handles the read paths. But I am not overly familiar with this code. |
The writing part is buried at the bottom: https://github.com/containers/image/pull/1506/files#diff-893e8c3da19873ab36cfae5eb73229819aed89b5115aca90e1aa7960ab661454R616-R624 |
|
Ok I missed the change in function name. LGTM |
vrothberg
force-pushed
the
fix-1365
branch
2 times, most recently
from
604dc1c
to
be483ce
Compare
|
Here we go ... the CI jobs are back |
| } | ||
|
|
||
| // Make sure to read-lock the file before reading it. | ||
| lock, err := lockfile.GetLockfile(path) |
There was a problem hiding this comment.
Isn’t the path parameter a path of the lock? Which contains a random value to support .Modified? How can auth.json be a JSON and a random string at the same time?
There was a problem hiding this comment.
Touch() and Modified() don't work in this scenario but I think that's OK in this scenario since we're controlling it here.
There was a problem hiding this comment.
Ah, I have not been reading that carefully enough; the contents are not actually used if we call neither Touch() nor Modified().
Still, that feels like an implementation detail that could change any time. I suppose our tests would catch it…
| @@ -552,6 +575,14 @@ func readJSONFile(path string, legacyFormat bool) (dockerConfigFile, error) { | |||
| return dockerConfigFile{}, err | |||
| } | |||
|
|
|||
| // When creating a file for the first time, it may be empty at this | |||
| // point as it was created by the write lock. Hence, return early here | |||
There was a problem hiding this comment.
That’s surprising to me (and incompatible with older versions of consumers, though that’s a small concern — they could easily race with WriteFile and see an empty file, and fail, for that reason).
See elsewhere about sharing the lock with JSON.
There was a problem hiding this comment.
I think it's a valid concern but I do not have a good/definitive answer/idea. We could use a central one (similar to the short-names lock) but old client would still compete with newer ones.
There was a problem hiding this comment.
I was thinking about using a separate auth.json.lock file; then the concerns about the lock file object trying to own file contents would go away, and so would this situation, AFAICS.
There was a problem hiding this comment.
The underlying fcntl lock is unlocked when any file descriptor to that file is closed, isn’t it?
If so, ioutil.ReadFile of the JSON always releases the lock, making the modifyJSON lock ineffective — and we do need to use a different path.
There was a problem hiding this comment.
I was thinking about using a separate
auth.json.lockfile; then the concerns about the lock file object trying to own file contents would go away, and so would this situation, AFAICS.
I was considering that but didn't like the user experience. I would find it surprising/annoying if I do a --authfile file and would find a file.lock afterwards.
The underlying
fcntllock is unlocked when any file descriptor to that file is closed, isn’t it?
Holy frog, that is true and buried in the man pages. That indeed is a K.O. for this approach - we could do ref-counting on the path but that seems a bit too hacky for my taste.
There was a problem hiding this comment.
Just to be explicit,pkg/lockfile is already using a global hash table and refcounting the objects to work correctly with those locks, so that works for exclusively-used lockfiles (and as long as c/storage doesn’t get a separate /v2 with a separate hash table); but it breaks if there is any other user of that file, like here.
There was a problem hiding this comment.
… the point about file.lock files on user-specified --authfile paths (outside of dot files, in user-maintained project repositories and the like) being annoying is quite valid, though; we might not even necessarily be able to create a lockfile (if the auth.json is in a read-only configuration directory).
flock(2) might be an option, but it is distinctly less portable (and the Linux man pages suggest that some implementations implement it on top of the fcntl locks. I’m sure there are good write-ups about the various locking mechanisms and their trade-offs, I haven’t now taken the time to find and read one.
There was a problem hiding this comment.
A possible partial alternative: use c/storage/pkg/ioutils.AtomicWriteFile. That could still lose data with concurrent writers, but at least it wouldn’t break readers concurrent with a single writer, who can now see empty files or partial contents (= invalid JSON).
There was a problem hiding this comment.
Seems like we're between a rock and hard place. I will close this PR as I ran out of time and really do not have a good idea yet.
There was a problem hiding this comment.
Let’s at least do the AtomicWriteFile part, that should be safe and make some difference: #1515 .
Read/Write lock each auth.json file before reading/writing it to prevent race coditions. Fixes: containers#1365 Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
|
@mtrmac updated the name and comment. Let's continue the conversation on the remaining locking issue in the threads. |
Read/Write lock each auth.json file before reading/writing
it to prevent race coditions.
Fixes: #1365
Signed-off-by: Valentin Rothberg vrothberg@redhat.com
@mtrmac @giuseppe @rhatdan PTAL