Race condition in SysVCacheItemPool constructor

[ericnorris]

I am using the CacheSessionPool in the Google Cloud Spanner PHP library for caching Spanner sessions.

In order to construct the CacheSessionPool, you are required to pass in a SysVCacheItemPool. Although the CacheSessionPool uses locking when interacting with the SysVCacheItemPool, there appears to be a race condition in __construct meaning you probably need to do locking yourself in order to create the object.

loadItems is called in __construct: https://github.com/googleapis/google-auth-library-php/blob/v1.4.0/src/Cache/SysVCacheItemPool.php#L135

loadItems then calls shm_attach: https://github.com/googleapis/google-auth-library-php/blob/v1.4.0/src/Cache/SysVCacheItemPool.php#L88-L92.

shm_attach attempts to 1) get the shared memory segment if it exists 2) create it if not: https://github.com/php/php-src/blob/PHP-7.2/ext/sysvshm/sysvshm.c#L171-L182.

If two processes attempt to construct a SysVCacheItemPool at the same time, see that there is not an existing shared memory segment, and then try to create one with IPC_CREAT | IPC_EXCL the WARNING on L178 (https://github.com/php/php-src/blob/PHP-7.2/ext/sysvshm/sysvshm.c#L177-L180) will occur.

Normally this may be okay, but I am running in an environment where I convert warnings to exceptions.

This could maybe be solved by not calling loadItems in the constructor, or I could file an issue in the https://github.com/googleapis/google-cloud-php repository to see what changes may need to be made there.

Environment details

• OS: Container-Optimized OS v75
• PHP version: PHP 7.2.11
• Package name and version:
  • google/auth@v1.4.0
  • google/cloud-spanner@1.11

Steps to reproduce

Not straightforward, since it is a race condition. You could try creating many SysVCacheItemPools in a tight loop in multiple processes.

[dwsupplee]

Thanks for the report @ericnorris, your proposal of moving loadItems out of the constructor seems reasonable to me. :)

It looks like we may just need to ensure we trigger loadItems in the delete/save methods as well if we go this route. Does this sound reasonable to you, @tmatsuo?

[tmatsuo]

Does moving loadItems really solve the problem? It will definitely solve the problem of the race condition in the constructor, but there will still be the same race condition in other places.

What about just ignore the warning by adding @ before the shm_attach as we do for @shm_get_var?

[tmatsuo]

Oh wait, if other places are safe because they're protected by lock, then that looks good to me to move it out from the constructor :)

[ericnorris]

Thanks for the quick response! Correct, the CacheSessionPool appears to properly handle locking, so the race conditions in non-constructor methods should be okay.

Relatedly, this would still be a problem for the FetchAuthTokenCache, however, since that doesn't use locking. Are there any plans to add locking to this, and should I create another issue if so?

I've noticed that the google-cloud-php library uses the FetchAuthTokenCache if you pass in the authCache configuration: https://github.com/googleapis/google-cloud-php/blob/9061f97983afa1bf947f5f5a4d92f99522259d24/Core/src/RequestWrapperTrait.php#L167-L171.

[dwsupplee]

Are there any plans to add locking to this

This isn't on our roadmap yet, but it is certainly something we can consider :). Another issue would be great.