Skip to content

Commit ab545d0

Browse files
smerasmera
committed
Merged PR 732516: Make sure enumerations are reported back when running front end related processes under a sandbox
Running graph construction tools under the frontend was missing directory enumerations, which was a reason for having underbuilds due to a wrongly cached build graph. Make sure we report these accesses and consume them appropriately.
1 parent 0c641fa commit ab545d0

File tree

9 files changed

+154
-8
lines changed

9 files changed

+154
-8
lines changed

‎Public/Src/Engine/ProcessPipExecutor/PipEnvironment.cs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,5 +174,10 @@ public static void ReportDuplicateVariable(LoggingContext loggingContext, string
174174
existingValue,
175175
ignoredValue);
176176
}
177+
178+
/// <summary>
179+
/// The OS-dependent environment variables that should be always present in a process environment.
180+
/// </summary>
181+
public IBuildParameters GetBaseEnvironmentVariables() => m_baseEnvironmentVariables;
177182
}
178183
}

‎Public/Src/FrontEnd/CMake/CMakeWorkspaceResolver.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ private async Task<Possible<Unit>> GenerateBuildDirectoryAsync()
185185
return new CMakeGenerationError(m_resolverSettings.ModuleName, m_buildDirectory.ToString(m_context.PathTable));
186186
}
187187

188-
FrontEndUtilities.TrackToolFileAccesses(m_host.Engine, m_context, Name, result.AllUnexpectedFileAccesses, outputDirectory);
188+
FrontEndUtilities.TrackToolFileAccesses(m_host.Engine, m_context, Name, result.AllUnexpectedFileAccesses.Union(result.ExplicitlyReportedFileAccesses), outputDirectory);
189189
return Possible.Create(Unit.Void);
190190
}
191191

‎Public/Src/FrontEnd/JavaScript/ToolBasedJavaScriptWorkspaceResolver.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,7 @@ protected virtual string GetProjectNameForGroup(IReadOnlyCollection<JavaScriptPr
202202
standardError);
203203
}
204204

205-
TrackFilesAndEnvironment(result.AllUnexpectedFileAccesses, outputFile.GetParent(Context.PathTable));
205+
TrackFilesAndEnvironment(result.AllUnexpectedFileAccesses.Union(result.ExplicitlyReportedFileAccesses), outputFile.GetParent(Context.PathTable));
206206

207207
JsonSerializer serializer = ConstructProjectGraphSerializer(JsonSerializerSettings);
208208

‎Public/Src/FrontEnd/MsBuild/MsBuildWorkspaceResolver.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ private async Task<Possible<ProjectGraphWithPredictionsResult<AbsolutePath>>> Co
348348
standardError);
349349
}
350350

351-
TrackFilesAndEnvironment(result.AllUnexpectedFileAccesses, outputFile.GetParent(Context.PathTable));
351+
TrackFilesAndEnvironment(result.AllUnexpectedFileAccesses.Union(result.ExplicitlyReportedFileAccesses), outputFile.GetParent(Context.PathTable));
352352
JsonSerializer serializer = ConstructProjectGraphSerializer(ProjectGraphSerializationSettings.Settings);
353353

354354
using (var sr = new StreamReader(outputFile.ToString(Context.PathTable)))

‎Public/Src/FrontEnd/Ninja/NinjaWorkspaceResolver.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
using Newtonsoft.Json;
2020
using TypeScript.Net.Types;
2121
using static BuildXL.Utilities.Core.FormattableStringEx;
22+
using System.Linq;
2223

2324
namespace BuildXL.FrontEnd.Ninja
2425
{
@@ -168,7 +169,7 @@ private async Task<Possible<NinjaGraphResult>> ComputeBuildGraphAsync()
168169
standardError);
169170
}
170171

171-
TrackFilesAndEnvironment(result.AllUnexpectedFileAccesses, outputFile.GetParent(Context.PathTable));
172+
TrackFilesAndEnvironment(result.AllUnexpectedFileAccesses.Union(result.ExplicitlyReportedFileAccesses), outputFile.GetParent(Context.PathTable));
172173
var serializer = JsonSerializer.Create(GraphSerializationSettings.Settings);
173174

174175
// Add custom deserializer for converting string arrays to AbsolutePath ReadOnlySets

‎Public/Src/FrontEnd/UnitTests/Utilities/FrontEndUtilitiesTest.cs‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT License.
3+
4+
using System.IO;
5+
using System.Linq;
6+
using BuildXL.FrontEnd.Sdk;
7+
using BuildXL.FrontEnd.Utilities;
8+
using BuildXL.Processes;
9+
using BuildXL.ProcessPipExecutor;
10+
using BuildXL.Utilities.Configuration;
11+
using BuildXL.Utilities.Configuration.Mutable;
12+
using BuildXL.Utilities.Core;
13+
using Test.BuildXL.Processes;
14+
using Test.BuildXL.TestUtilities.Xunit;
15+
using Xunit;
16+
using Xunit.Abstractions;
17+
18+
namespace Test.BuildXL.FrontEnd.Utilities
19+
{
20+
public class FrontEndUtilitiesTest : TemporaryStorageTestBase
21+
{
22+
protected BuildXLContext Context { get; }
23+
protected FrontEndContext FrontEndContext { get; }
24+
protected IConfiguration Configuration { get; }
25+
protected PipEnvironment PipEnvironment { get; }
26+
27+
public FrontEndUtilitiesTest(ITestOutputHelper output) : base(output)
28+
{
29+
Configuration = new ConfigurationImpl();
30+
Context = BuildXLContext.CreateInstanceForTesting();
31+
FrontEndContext = FrontEndContext.CreateInstanceForTesting(
32+
pathTable: Context.PathTable, symbolTable: Context.SymbolTable, qualifierTable: Context.QualifierTable, frontEndConfig: Configuration.FrontEnd, loggingContext: LoggingContext);
33+
PipEnvironment = new PipEnvironment(LoggingContext);
34+
}
35+
36+
[Fact]
37+
public void RunSandboxedToolHasAllExpectedAccesses()
38+
{
39+
var input = GetFullPath("input");
40+
var inputPath = AbsolutePath.Create(Context.PathTable, input);
41+
File.WriteAllText(input, "some input");
42+
43+
var output = GetFullPath("output");
44+
var outputPath = AbsolutePath.Create(Context.PathTable, output);
45+
46+
var temporaryDirectoryPath = AbsolutePath.Create(Context.PathTable, TemporaryDirectory);
47+
var nonExistentPath = AbsolutePath.Create(Context.PathTable, GetFullPath("non-existent"));
48+
49+
string toolArguments;
50+
51+
// Let's test a read, an enumeration, a write and an absent probe
52+
if (OperatingSystemHelper.IsWindowsOS)
53+
{
54+
toolArguments = "/C \"type input & dir & copy input output & if exist non-existent echo hi\"";
55+
}
56+
else
57+
{
58+
toolArguments = "-c \"cat input; ls; cp input output; if [ -e non-existent ]; then echo hi; fi\"";
59+
}
60+
61+
var toolDirectory = AbsolutePath.Create(Context.PathTable, CmdHelper.OsShellExe).GetParent(Context.PathTable);
62+
63+
var result = FrontEndUtilities.RunSandboxedToolAsync(
64+
FrontEndContext,
65+
CmdHelper.OsShellExe,
66+
buildStorageDirectory: TemporaryDirectory,
67+
fileAccessManifest: FrontEndUtilities.GenerateToolFileAccessManifest(FrontEndContext, toolDirectory),
68+
arguments: toolArguments,
69+
workingDirectory: TemporaryDirectory,
70+
description: $"Test sandboxed tool",
71+
PipEnvironment.GetBaseEnvironmentVariables()).GetAwaiter().GetResult();
72+
73+
XAssert.AreEqual(0, result.ExitCode);
74+
var allAccesses = result.AllUnexpectedFileAccesses.Union(result.ExplicitlyReportedFileAccesses);
75+
76+
// type input
77+
XAssert.IsTrue(allAccesses.Any(a => a.RequestedAccess == RequestedAccess.Read && GetAbsolutePath(a) == inputPath));
78+
// dir
79+
XAssert.IsTrue(allAccesses.Any(a => a.RequestedAccess == RequestedAccess.Enumerate && GetAbsolutePath(a) == temporaryDirectoryPath));
80+
// copy input output
81+
XAssert.IsTrue(allAccesses.Any(a => a.RequestedAccess == RequestedAccess.Write && GetAbsolutePath(a) == outputPath));
82+
// if exist non-existent echo hi
83+
XAssert.IsTrue(allAccesses.Any(a => a.RequestedAccess == RequestedAccess.Probe && GetAbsolutePath(a) == nonExistentPath));
84+
}
85+
86+
private AbsolutePath GetAbsolutePath(ReportedFileAccess access) => AbsolutePath.Create(Context.PathTable, access.GetPath(Context.PathTable));
87+
}
88+
}

‎Public/Src/FrontEnd/UnitTests/Utilities/Test.BuildXL.FrontEnd.Utilities.dsc‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
// Copyright (c) Microsoft. All rights reserved.
2+
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
3+
4+
import * as Managed from "Sdk.Managed";
5+
import * as MSBuild from "Sdk.Selfhost.MSBuild";
6+
import * as Frameworks from "Sdk.Managed.Frameworks";
7+
import {Node} from "Sdk.NodeJs";
8+
import {Transformer} from "Sdk.Transformers";
9+
10+
namespace Test.Utilities {
11+
12+
@@public
13+
export const dll = BuildXLSdk.test({
14+
runTestArgs: {
15+
unsafeTestRunArguments: {
16+
// These tests require Detours to run itself, so we won't detour the test runner process itself
17+
runWithUntrackedDependencies: true
18+
},
19+
},
20+
assemblyName: "Test.BuildXL.FrontEnd.Utilities",
21+
sources: globR(d`.`, "*.cs"),
22+
references: [
23+
Script.dll,
24+
Core.dll,
25+
importFrom("BuildXL.Core.UnitTests").EngineTestUtilities.dll,
26+
importFrom("BuildXL.Engine").Engine.dll,
27+
importFrom("BuildXL.Engine").Processes.dll,
28+
importFrom("BuildXL.Engine").Scheduler.dll,
29+
importFrom("BuildXL.Engine").ProcessPipExecutor.dll,
30+
importFrom("BuildXL.FrontEnd").Core.dll,
31+
importFrom("BuildXL.FrontEnd").Script.dll,
32+
importFrom("BuildXL.FrontEnd").Sdk.dll,
33+
importFrom("BuildXL.FrontEnd").JavaScript.dll,
34+
importFrom("BuildXL.FrontEnd").TypeScript.Net.dll,
35+
importFrom("BuildXL.FrontEnd").Utilities.dll,
36+
importFrom("BuildXL.FrontEnd").SdkProjectGraph.dll,
37+
importFrom("BuildXL.Pips").dll,
38+
importFrom("BuildXL.Utilities").dll,
39+
importFrom("BuildXL.Utilities").Configuration.dll,
40+
importFrom("BuildXL.Utilities").Native.dll,
41+
importFrom("BuildXL.Utilities").Script.Constants.dll,
42+
importFrom("BuildXL.Utilities").Utilities.Core.dll,
43+
],
44+
});
45+
}

‎Public/Src/FrontEnd/Utilities/FrontEndUtilities.cs‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,12 @@ public static IDictionary<string, string> GetEngineEnvironment(FrontEndEngineAbs
246246
/// <summary>
247247
/// Generate a basic file access manifest for front end tools
248248
/// </summary>
249+
/// <remarks>
250+
/// The generated manifest will instruct the sandbox to not fail on unexpected file accesses, but to deny all of them.
251+
/// Therefore, if the goal is collecting all file accesses, check <see cref="SandboxedProcessResult.AllUnexpectedFileAccesses"/> but
252+
/// also check <see cref="SandboxedProcessResult.ExplicitlyReportedFileAccesses"/>, since accesses like enumerations
253+
/// are never denied.
254+
/// </remarks>
249255
public static FileAccessManifest GenerateToolFileAccessManifest(FrontEndContext context, AbsolutePath toolDirectory)
250256
{
251257
var pathTable = context.PathTable;
@@ -255,8 +261,6 @@ public static FileAccessManifest GenerateToolFileAccessManifest(FrontEndContext
255261
var fileAccessManifest = new FileAccessManifest(pathTable)
256262
{
257263
FailUnexpectedFileAccesses = false,
258-
// The manifest is configured such that all relevant accesses are returned as unexpected ones, so we
259-
// don't actually need to return all accesses
260264
ReportFileAccesses = false,
261265
MonitorNtCreateFile = true,
262266
MonitorZwCreateOpenQueryFile = true,
@@ -285,14 +289,17 @@ public static FileAccessManifest GenerateToolFileAccessManifest(FrontEndContext
285289

286290
fileAccessManifest.AddScope(toolDirectory, FileAccessPolicy.MaskAll, FileAccessPolicy.AllowReadAlways);
287291

292+
// Make sure the root node is configured so all accesses are reported
293+
fileAccessManifest.AddScope(AbsolutePath.Invalid, FileAccessPolicy.MaskNothing, FileAccessPolicy.ReportAccess);
294+
288295
return fileAccessManifest;
289296
}
290297

291298
/// <summary>
292299
/// The FrontEnds that use an out-of-proc tool should sandbox that process and call this method
293300
/// in order to tack the tool's file accesses, enumerations, etc. in order to make graph caching sound
294301
/// </summary>
295-
public static void TrackToolFileAccesses(FrontEndEngineAbstraction engine, FrontEndContext context, string frontEndName, ISet<ReportedFileAccess> fileAccesses, AbsolutePath frontEndFolder)
302+
public static void TrackToolFileAccesses(FrontEndEngineAbstraction engine, FrontEndContext context, string frontEndName, IEnumerable<ReportedFileAccess> fileAccesses, AbsolutePath frontEndFolder)
296303
{
297304
// Compute all parseable paths
298305
// TODO: does it make sense to consider enumerations, or as a result the graph will be too unstable? Does it matter for MsBuild graph construction?

‎Public/Src/FrontEnd/Utilities/GenericProjectGraphResolver/ProjectGraphWorkspaceResolverBase.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,7 @@ protected BuildParameters.IBuildParameters RetrieveBuildParameters()
276276
/// <summary>
277277
/// Talks to the engine to register all accesses and build parameters
278278
/// </summary>
279-
protected void TrackFilesAndEnvironment(ISet<ReportedFileAccess> fileAccesses, AbsolutePath frontEndFolder)
279+
protected void TrackFilesAndEnvironment(IEnumerable<ReportedFileAccess> fileAccesses, AbsolutePath frontEndFolder)
280280
{
281281
// Register all build parameters passed to the graph construction process if they were retrieved from the process environment
282282
// Otherwise, if build parameters were defined by the main config file, then there is nothing to register: if the definition

0 commit comments

Comments
 (0)