Skip to content

Commit

Permalink
Fix potential race in Spawn#getLocalResources()
Browse files Browse the repository at this point in the history
Both implementations performed two reads of the nullable instance variable, which could be reordered so that the first read returns a non-null value and the second read returns null.

Closes #24985.

PiperOrigin-RevId: 719418766
Change-Id: Ib01b0a86c7d0e61f3e2f14bc6520c375b456d086
  • Loading branch information
fmeum authored and copybara-github committed Jan 24, 2025
1 parent ddd7be9 commit e66d61d
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public class BaseSpawn implements Spawn {
private final ImmutableMap<String, String> executionInfo;
private final ActionExecutionMetadata action;
private final ResourceSetOrBuilder localResources;
private ResourceSet localResourcesCached = null;
@Nullable private ResourceSet localResourcesCached;

public BaseSpawn(
List<String> arguments,
Expand Down Expand Up @@ -92,13 +92,15 @@ public ActionExecutionMetadata getResourceOwner() {

@Override
public ResourceSet getLocalResources() throws ExecException {
if (localResourcesCached == null) {
ResourceSet result = localResourcesCached;
if (result == null) {
// Not expected to be called concurrently, and an idempotent computation if it is.
localResourcesCached =
result =
localResources.buildResourceSet(
OS.getCurrent(), action.getInputs().memoizedFlattenAndGetSize());
localResourcesCached = result;
}
return localResourcesCached;
return result;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public final class SimpleSpawn implements Spawn {
@Nullable private final Set<? extends ActionInput> mandatoryOutputs;
private final PathMapper pathMapper;
private final LocalResourcesSupplier localResourcesSupplier;
private ResourceSet localResourcesCached;
@Nullable private ResourceSet localResourcesCached;

private SimpleSpawn(
ActionExecutionMetadata owner,
Expand Down Expand Up @@ -278,11 +278,13 @@ public ActionExecutionMetadata getResourceOwner() {

@Override
public ResourceSet getLocalResources() throws ExecException {
if (localResourcesCached == null) {
ResourceSet result = localResourcesCached;
if (result == null) {
// Not expected to be called concurrently, and an idempotent computation if it is.
localResourcesCached = localResourcesSupplier.get();
result = localResourcesSupplier.get();
localResourcesCached = result;
}
return localResourcesCached;
return result;
}

@Override
Expand Down

0 comments on commit e66d61d

Please sign in to comment.