Skip to content

The file_put_contents() is not atomic and can cause race in file caching #740

@mikkorantalainen

Description

@mikkorantalainen

The problematic code here is here:

return (bool) file_put_contents($this->name, $data);

The problem with that code is that if two PHP processes fetch the file at the same time and use file_put_contents() to update the cache, the end result can be interleaved data from both processes which obviously results in invalid cache file which then will later fail when loaded with file_get_contents() and unserialize()d. This race is pretty hard to reproduce because file_put_contents() is basically combination of open()+write()+close() and the race happens if second process can execute open() before write() is called in first process but I think it happened at least once for our service.

I think something like following would be safe:

-                       return (bool) file_put_contents($this->name, $data);
+                       $tmpname = $this->name.".".gethostname()."-".getmypid();
+                       $rv = (bool) file_put_contents($tmpname, $data);
+                       if (false === $rv)
+                               return false;
+                       $rv = rename($tmpname, $this->name);
+                       return $rv;

This includes gethostname() to make the execution still atomic if cache is backed by NFS and two hosts running SimplePie happened to be unlucky enough to use the same PID, too. As far as I know, if the backend is NFS v2 there's no way to get filesystem to reserve atomic unique temporary filename so suitable filenames should be used instead. Even with old NFS backends, rename() is atomic meaning either the old version is active or new version is active but there can never be mixed case in between.

Basically the above implementation writes data to temporary filename first where temporary filename includes hostname and PID of the currently running process. This should result in a filename that cannot ever collide with concurrent PHP process and the rename() is atomic resulting in one intact cache file to remain in use.

This implementation would leave the stale temporary file if rename() fails for any reason. I'm not sure if that's good or bad. One could try to call unlink() for the temporary file if rename() fails but I'd expect rename() fail only in case of read-only filesystem and unlink() would then obviously fail, too. However, if you assume permissions of existing cache file has been set incorrectly, then rename() might fail because of file permissions – leaving the temporary file around in this case might be beneficial if admin were then to fix the issue manually.

And I'm not sure if gethostname() can actually fail to return a string in practice. According to the documentation it can return false. That would catenate as empty string so it should be still okay in the above implementation but that could result in identical temporary filename used on NFS backend.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions