Merged PR 741487: Make execute permission configurable and disable it for BXL internal repo

Introduced a flag to  enable/disable explicit setting of the execute permissions bit for the root process in linux builds.
Added a condition to explicitly set this bit for node and dotnet in the extractor.
This is done to obtain more information about the linux permissions bug.

Related work items: #2104538
This commit is contained in:
Sahiti Chandramouli 2023-10-03 02:09:45 +00:00
Родитель fc6f3396e9
Коммит e855dfe58a
17 изменённых файлов: 92 добавлений и 62 удалений

Просмотреть файл

@ -96,7 +96,7 @@ jobs:
# This step currently only builds selfhost with the --minimal flag, but will be extended in the future to run more unit tests with ptrace
- bash: |
set -eu
bash bxl.sh /ado /cacheMiss:"[Bxl.Selfhost.Linux.PTrace]" /logObservedFileAccesses /cacheConfigFilePath:Out/CacheConfig.json /logoutput:FullOutputOnError /logsToRetain:10 /exp:lazysodeletion- /logsDirectory:"Out/Logs/Build" --minimal --internal --use-dev "/p:BUILDXL_FINGERPRINT_SALT='Bxl.Selfhost.Linux.PTrace.CasingPR'" /forceEnableLinuxPTraceSandbox+ /injectCacheMisses:0.3
bash bxl.sh /ado /cacheMiss:"[Bxl.Selfhost.Linux.PTrace]" /logObservedFileAccesses /cacheConfigFilePath:Out/CacheConfig.json /logoutput:FullOutputOnError /logsToRetain:10 /exp:lazysodeletion- /logsDirectory:"Out/Logs/Build" --minimal --internal --use-dev /forceEnableLinuxPTraceSandbox+ /injectCacheMisses:0.3
displayName: Build BXL with LKG and PTrace
workingDirectory: /home/subst
env:

Просмотреть файл

@ -149,7 +149,7 @@ jobs:
# - the disks on Azure Pipeline VMs are too small to build everything, so let's instead run tests
# - we also disable early worker release to avoid releasing a worker before attachment, which tends to happen
# when the build is highly cached: the intention is to have as much of a distributed build as possible for validation purposes
./bxl.sh --use-dev --use-adobuildrunner ${{ parameters.BxlCommonArgs }} /logsDirectory:"Out/Logs/${{ parameters.validationName }}" ${{ parameters.bxlExtraArgs }} "/f:tag='test' /p:BUILDXL_FINGERPRINT_SALT='PrPipelineSalt_20230825'" /earlyWorkerRelease- /p:BuildXLWorkerAttachTimeoutMin=10 /logToKusto /logToKustoBlobUri:https://adomessages.blob.core.windows.net/adomessages /logToKustoIdentityId:6e0959cf-a9ba-4988-bbf1-7facd9deda51 /logToKustoTenantId:975f013f-7f24-47e8-a7d3-abc4752bf346 /historicMetadataCache-
./bxl.sh --use-dev --use-adobuildrunner ${{ parameters.BxlCommonArgs }} /logsDirectory:"Out/Logs/${{ parameters.validationName }}" ${{ parameters.bxlExtraArgs }} "/f:tag='test'" /earlyWorkerRelease- /p:BuildXLWorkerAttachTimeoutMin=10 /logToKusto /logToKustoBlobUri:https://adomessages.blob.core.windows.net/adomessages /logToKustoIdentityId:6e0959cf-a9ba-4988-bbf1-7facd9deda51 /logToKustoTenantId:975f013f-7f24-47e8-a7d3-abc4752bf346 /historicMetadataCache-
displayName: Test (${{ parameters.validationName }})
workingDirectory: /home/subst
env:

Просмотреть файл

@ -381,6 +381,9 @@ namespace BuildXL
OptionHandlerFactory.CreateBoolOption(
"disableIsObsoleteCheckDuringConversion",
sign => frontEndConfiguration.DisableIsObsoleteCheckDuringConversion = sign),
OptionHandlerFactory.CreateBoolOption(
"forceAddExecutionPermission",
sign => sandboxConfiguration.ForceAddExecutionPermission = sign),
OptionHandlerFactory.CreateOption2(
"distributedBuildRole",
"dbr",

Просмотреть файл

@ -922,6 +922,12 @@ namespace BuildXL
HelpLevel.Verbose
);
hw.WriteOption(
"/forceAddExecutionPermission[+|-]",
Strings.HelpText_DisplayHelp_ForceAddExecutionPermission,
HelpLevel.Verbose
);
#endregion
hw.WriteBanner(

Просмотреть файл

@ -1168,4 +1168,7 @@ Example: ad2d42d2ec5d2ca0c0b7ad65402d07c7ef40b91e</value>
<data name="HelpText_DisplayHelp_EnableVerboseProcessLogging" xml:space="preserve">
<value>Enables verbose sandbox logging for specific pips based on their formatted semistable hashes. This is tantamount to switching /logObservedFileAccesses and /logProcesses for these pips, and also enabling verbose debug logging in the sandbox. A list of semistable hashes might be specified, separated by semicolons. If the special value "*" is given, sandbox logging will be enabled for every pip. Example: /debug_enableVerboseProcessLogging:Pip3855A4C7E1E820D0;PipE9638AD7DDD6AF67</value>
</data>
<data name="HelpText_DisplayHelp_ForceAddExecutionPermission" xml:space="preserve">
<value>When set to true, it enables the execution permission for the root process of process pips in Linux builds. Defaults to true.</value>
</data>
</root>

Просмотреть файл

@ -837,7 +837,8 @@ namespace BuildXL.ProcessPipExecutor
sandboxConnection: sandboxConnection,
sidebandWriter: sidebandWriter,
detoursEventListener: m_detoursListener,
fileSystemView: fileSystemView)
fileSystemView: fileSystemView,
forceAddExecutionPermission: m_sandboxConfig.ForceAddExecutionPermission)
{
Arguments = arguments,
WorkingDirectory = m_workingDirectory,

Просмотреть файл

@ -132,7 +132,8 @@ namespace BuildXL.Processes
IDetoursEventListener? detoursEventListener = null,
ISandboxConnection? sandboxConnection = null,
bool createJobObjectForCurrentProcess = true,
SandboxedProcessResourceMonitoringConfig? monitoringConfig = null)
SandboxedProcessResourceMonitoringConfig? monitoringConfig = null,
bool forceAddExecutionPermission = true)
: this(
new PathTable(),
fileStorage,
@ -143,7 +144,8 @@ namespace BuildXL.Processes
detoursEventListener,
sandboxConnection,
createJobObjectForCurrentProcess: createJobObjectForCurrentProcess,
monitoringConfig: monitoringConfig)
monitoringConfig: monitoringConfig,
forceAddExecutionPermission: forceAddExecutionPermission)
{
}
@ -163,7 +165,8 @@ namespace BuildXL.Processes
SidebandWriter? sidebandWriter = null,
bool createJobObjectForCurrentProcess = true,
ISandboxFileSystemView? fileSystemView = null,
SandboxedProcessResourceMonitoringConfig? monitoringConfig = null)
SandboxedProcessResourceMonitoringConfig? monitoringConfig = null,
bool forceAddExecutionPermission = true)
{
PathTable = pathTable;
FileAccessManifest = fileAccessManifest ?? new FileAccessManifest(pathTable);
@ -182,6 +185,7 @@ namespace BuildXL.Processes
CreateJobObjectForCurrentProcess = createJobObjectForCurrentProcess;
FileSystemView = fileSystemView;
MonitoringConfig = monitoringConfig;
ForceAddExecutionPermission = forceAddExecutionPermission;
}
/// <summary>
@ -198,7 +202,8 @@ namespace BuildXL.Processes
ISandboxConnection? sandboxConnection = null,
FileAccessManifest? fileAccessManifest = null,
bool createJobObjectForCurrentProcess = true,
SandboxedProcessResourceMonitoringConfig? monitoringConfig = null
SandboxedProcessResourceMonitoringConfig? monitoringConfig = null,
bool forceAddExecutionPermission = true
)
: this(
pathTable,
@ -211,7 +216,8 @@ namespace BuildXL.Processes
detoursEventListener,
sandboxConnection,
createJobObjectForCurrentProcess: createJobObjectForCurrentProcess,
monitoringConfig: monitoringConfig)
monitoringConfig: monitoringConfig,
forceAddExecutionPermission: forceAddExecutionPermission)
{
}
@ -291,6 +297,11 @@ namespace BuildXL.Processes
/// </remarks>
public int NumRetriesPipeReadOnCancel { get; set; } = DefaultPipeReadRetryOnCancellationCount;
/// <summary>
/// Force set the execute permission bit for the root process of process pips in Linux builds.
/// </summary>
public bool ForceAddExecutionPermission { get; }
/// <summary>
/// Encoded command line arguments
/// </summary>
@ -595,6 +606,7 @@ namespace BuildXL.Processes
writer.WriteNullableString(DetoursFailureFile);
writer.WriteNullableReadOnlyList(ExternalVmSandboxStaleFilesToClean, (w, s) => w.Write(s));
writer.Write(CreateSandboxTraceFile);
writer.Write(ForceAddExecutionPermission);
// File access manifest should be serialized the last.
writer.Write(FileAccessManifest, (w, v) => FileAccessManifest.Serialize(stream));
@ -643,9 +655,9 @@ namespace BuildXL.Processes
var detoursFailureFile = reader.ReadNullableString();
var externalVmSandboxStaleFilesToClean = reader.ReadNullableReadOnlyList(r => r.ReadString());
var createSandboxTraceFile = reader.ReadBoolean();
bool forceAddExecutionPermission = reader.ReadBoolean();
var fam = reader.ReadNullable(r => FileAccessManifest.Deserialize(stream));
return new SandboxedProcessInfo(
new PathTable(),
sandboxedProcessStandardFiles != null ? new StandardFileStorage(sandboxedProcessStandardFiles) : null,
@ -655,7 +667,8 @@ namespace BuildXL.Processes
loggingContext: loggingContext,
sidebandWriter: sidebandWritter,
detoursEventListener: detoursEventListener,
createJobObjectForCurrentProcess: createJobObjectForCurrentProcess)
createJobObjectForCurrentProcess: createJobObjectForCurrentProcess,
forceAddExecutionPermission: forceAddExecutionPermission)
{
m_arguments = arguments,
m_commandLine = commandLine,

Просмотреть файл

@ -213,8 +213,11 @@ namespace BuildXL.Processes
DegreeOfParallelism: 1, // Must be one, otherwise SandboxedPipExecutor will fail asserting valid reports
SingleProducerConstrained: useSingleProducer);
m_pendingReports = ActionBlockSlim.Create<AccessReport>(configuration: executionOptions, HandleAccessReport);
m_pendingReports = ActionBlockSlim.Create<AccessReport>(configuration: executionOptions,
(accessReport) =>
{
HandleAccessReport(accessReport, info.ForceAddExecutionPermission);
});
// install 'ProcessReady' and 'ProcessStarted' handlers to inform the sandbox
ProcessReady += () => SandboxConnection.NotifyPipReady(info.LoggingContext, info.FileAccessManifest, this, m_pendingReports.Completion);
ProcessStarted += (pid) => OnProcessStartedAsync(info).GetAwaiter().GetResult();
@ -280,7 +283,10 @@ namespace BuildXL.Processes
{
// This is a workaround for an issue that appears on Linux where some executables in the BuildXL
// nuget/npm package may not have the execute bit set causing a permission denied error
_ = FileUtilities.TrySetExecutePermissionIfNeeded(process.StartInfo.FileName);
if (info.ForceAddExecutionPermission)
{
_ = FileUtilities.TrySetExecutePermissionIfNeeded(process.StartInfo.FileName);
}
process.StartInfo.Arguments = $"{process.StartInfo.FileName} {process.StartInfo.Arguments}";
process.StartInfo.FileName = EnvExecutable;
@ -602,7 +608,7 @@ namespace BuildXL.Processes
#endif
}
private async Task FeedStdInAsync(SandboxedProcessInfo info, string? processStdinFileName)
private async Task FeedStdInAsync(SandboxedProcessInfo info, string? processStdinFileName, bool forceAddExecutionPermission = true)
{
Contract.Requires(info.RootJailInfo == null || !NeedsShellWrapping(), "Cannot run root jail on this OS");
@ -651,7 +657,10 @@ namespace BuildXL.Processes
lines.Add($"exec {cmdLine}");
SetExecutePermissionIfNeeded(info.FileName, throwIfNotFound: false);
if (info.ForceAddExecutionPermission)
{
SetExecutePermissionIfNeeded(info.FileName, throwIfNotFound: false);
}
foreach (string line in lines)
{
await Process.StandardInput.WriteLineAsync(line);
@ -836,7 +845,7 @@ namespace BuildXL.Processes
m_sumOfReportQueueTimesUs += (stats.DequeueTime - stats.EnqueueTime) / 1000;
}
private void HandleAccessReport(AccessReport report)
private void HandleAccessReport(AccessReport report, bool forceAddExecutionPermission)
{
if (ShouldReportFileAccesses && report.Operation != FileOperation.OpDebugMessage)
{
@ -869,7 +878,7 @@ namespace BuildXL.Processes
// The process is statically linked, a ptrace runner needs to be started up
if (report.Operation == FileOperation.OpStaticallyLinkedProcess)
{
StartPTraceRunner(report.Pid, reportPath);
StartPTraceRunner(report.Pid, reportPath, forceAddExecutionPermission);
}
var pathExists = true;
@ -1067,7 +1076,7 @@ namespace BuildXL.Processes
I($"{operation}:{pid}|{requestedAccess}|{status}|{explicitLogging}|{error}|{path}|{isDirectory}|e:{report.Statistics.EnqueueTime}|h:{processTime}us|q:{queueTime}us");
}
private void StartPTraceRunner(int pid, string path)
private void StartPTraceRunner(int pid, string path, bool forceAddExecutionPermission)
{
var paths = SandboxConnectionLinuxDetours.GetPaths(RootJailInfo, UniqueName);
var args = $"-c {pid} -x {path}";
@ -1094,8 +1103,9 @@ namespace BuildXL.Processes
// We will kill these manually if the pip is exiting
Timeout.InfiniteTimeSpan,
// The runner will only log to stderr if there's a problem, other logs go to the main log using the fifo
errorBuilder: line => { if (line != null) { Logger.Log.PTraceRunnerError(m_loggingContext, line); } }
);
errorBuilder: line => { if (line != null) { Logger.Log.PTraceRunnerError(m_loggingContext, line); } },
forceAddExecutionPermission: forceAddExecutionPermission
);
ptraceRunner.Start();
m_ptraceRunners.Add(runnerTask(ptraceRunner));

Просмотреть файл

@ -182,7 +182,8 @@ namespace BuildXL.Processes
{
LogProcessState($"Unable to generate core dump: {m_dumpCreationException.GetLogEventMessage()}");
}
});
},
forceAddExecutionPermission: info.ForceAddExecutionPermission);
}
/// <summary>

Просмотреть файл

@ -75,12 +75,6 @@ namespace Test.BuildXL.FrontEnd.Lage
SourceRoot = Path.Combine(TestRoot, RelativeSourceRoot);
OutDir = "target";
// TODO Bug 2073919- this is a temporary workaround to tests that are flakey due to the execute permission
// not correctly transiting through cache
AssertTrue(FileUtilities.TrySetExecutePermissionIfNeeded(PathToYarn).Succeeded);
AssertTrue(FileUtilities.TrySetExecutePermissionIfNeeded(PathToLage).Succeeded);
AssertTrue(FileUtilities.TrySetExecutePermissionIfNeeded(PathToNode).Succeeded);
}
/// <nodoc/>

Просмотреть файл

@ -80,11 +80,6 @@ namespace Test.BuildXL.FrontEnd.Rush
// Make sure the user profile and temp folders exist
Directory.CreateDirectory(RushUserProfile);
Directory.CreateDirectory(RushTempFolder);
// TODO Bug 2073919- this is a temporary workaround to tests that are flakey due to the execute permission
// not correctly transiting through cache
AssertTrue(FileUtilities.TrySetExecutePermissionIfNeeded(PathToRush).Succeeded);
AssertTrue(FileUtilities.TrySetExecutePermissionIfNeeded(PathToNode).Succeeded);
}
protected SpecEvaluationBuilder Build(

Просмотреть файл

@ -67,11 +67,6 @@ namespace Test.BuildXL.FrontEnd.Yarn
SourceRoot = Path.Combine(TestRoot, RelativeSourceRoot);
OutDir = "target";
// TODO Bug 2073919- this is a temporary workaround to tests that are flakey due to the execute permission
// not correctly transiting through cache
AssertTrue(FileUtilities.TrySetExecutePermissionIfNeeded(PathToYarn).Succeeded);
AssertTrue(FileUtilities.TrySetExecutePermissionIfNeeded(PathToNode).Succeeded);
}
protected SpecEvaluationBuilder Build(

Просмотреть файл

@ -25,7 +25,7 @@ namespace Tool.Download
private static readonly Dictionary<string, string> packagesToBeChecked = new Dictionary<string, string>
{
{"NodeJs.linux-x64", "node-v18.6.0-linux-x64/bin/node"},
{"YarnTool", "yarn-v1.22.19/bin/yarn"}
{"DotNet-Runtime.linux", "dotnet"}
};
private Extractor() : base("Extractor")
@ -141,7 +141,7 @@ namespace Tool.Download
{
if (target.Contains(packageName))
{
if (!CheckForNodePermissions(target, packagesToBeChecked[packageName]))
if (!SetExecutePermissionsForExtractedFiles(target, packagesToBeChecked[packageName]))
{
return false;
}
@ -197,34 +197,21 @@ namespace Tool.Download
}
/// <summary>
/// This method is used to check if the execute permission bit has been set or not.
/// This method is used to set the execute permissions bit for the extracted files.
/// </summary>
/// <remarks>
/// In the method below we are checking this specifically for Node package in linux, as that was causing the issue.
/// In the method below we are adding the bit specifically for Node and Dotnet package in linux, as they are causing the issue.
/// This is only set for the BuildXL.Internal repo build and is not expected to kick in for end user builds.
/// It is expected to be shortlived and probably generalized to setting the execute permission for all extractor output.
/// TODO: Need to remove this hack once the bug is fixed. Refer bug https://dev.azure.com/mseng/1ES/_workitems/edit/2073919 for further information.
/// </remarks>
private bool CheckForNodePermissions(string target, string relativePath)
private bool SetExecutePermissionsForExtractedFiles(string target, string relativePath)
{
string fullPathForExecutableFile = Path.Combine(target, relativePath);
if (File.Exists(fullPathForExecutableFile))
{
var mode = GetFilePermissionsForFilePath(fullPathForExecutableFile, false);
if (mode < 0)
{
Console.Error.WriteLine($"Failed to retrieve file permissions for : {fullPathForExecutableFile}");
return false;
}
else
{
// Check if the execute file permission bit has been set or not.
var filePermissions = checked((FilePermissions)mode);
if ((filePermissions & FilePermissions.S_IXUSR) == FilePermissions.S_IXUSR)
{
Console.Error.WriteLine($"File : {fullPathForExecutableFile} does not have the execute bit set for the user account");
return false;
}
}
_ = FileUtilities.TrySetExecutePermissionIfNeeded(fullPathForExecutableFile);
}
else
{

Просмотреть файл

@ -339,5 +339,10 @@ namespace BuildXL.Utilities.Configuration
/// Ignores DeviceIoControl calls, in particular the case of FSCTL_GET_REPARSE_POINT
/// </summary>
bool IgnoreDeviceIoControlGetReparsePoint { get; }
/// <summary>
/// Force set the execute permission bit for the root process of process pips in Linux builds.
/// </summary>
public bool ForceAddExecutionPermission { get; }
}
}

Просмотреть файл

@ -58,6 +58,7 @@ namespace BuildXL.Utilities.Configuration.Mutable
UnconditionallyEnableLinuxPTraceSandbox = false;
// TODO: flip the default once we have verified this is not a breaking change
IgnoreDeviceIoControlGetReparsePoint = true;
ForceAddExecutionPermission = true;
}
/// <nodoc />
@ -114,6 +115,7 @@ namespace BuildXL.Utilities.Configuration.Mutable
AlwaysRemoteInjectDetoursFrom32BitProcess = template.AlwaysRemoteInjectDetoursFrom32BitProcess;
UnconditionallyEnableLinuxPTraceSandbox = template.UnconditionallyEnableLinuxPTraceSandbox;
IgnoreDeviceIoControlGetReparsePoint = template.IgnoreDeviceIoControlGetReparsePoint;
ForceAddExecutionPermission = template.ForceAddExecutionPermission;
}
/// <inheritdoc />
@ -299,5 +301,8 @@ namespace BuildXL.Utilities.Configuration.Mutable
/// <inheritdoc/>
public bool IgnoreDeviceIoControlGetReparsePoint { get; set; }
/// <inheritdoc/>
public bool ForceAddExecutionPermission { get; set; }
}
}

Просмотреть файл

@ -80,6 +80,11 @@ namespace BuildXL.Utilities.Core
private readonly Action<string> m_outputBuilder;
private readonly Action<string> m_errorBuilder;
/// <summary>
/// Force set the execute permission bit for the root process of process pips in Linux builds.
/// </summary>
private readonly bool m_forceAddExecutionPermission;
private int m_processId = -1;
private int GetProcessIdSafe()
@ -136,7 +141,8 @@ namespace BuildXL.Utilities.Core
Action<string> errorBuilder = null,
string provenance = null,
Action<string> logger = null,
Action dumpProcessTree = null)
Action dumpProcessTree = null,
bool forceAddExecutionPermission = true)
{
Contract.RequiresNotNull(process);
@ -160,6 +166,7 @@ namespace BuildXL.Utilities.Core
m_timeout = timeout;
m_provenance = provenance;
m_dumpProcessTree = dumpProcessTree;
m_forceAddExecutionPermission = forceAddExecutionPermission;
}
/// <summary>
@ -217,7 +224,11 @@ namespace BuildXL.Utilities.Core
try
{
SetExecutePermissionIfNeeded(Process.StartInfo.FileName);
if (m_forceAddExecutionPermission)
{
SetExecutePermissionIfNeeded(Process.StartInfo.FileName);
}
Process.Start();
}
catch (Win32Exception e)

5
bxl.sh
Просмотреть файл

@ -326,9 +326,10 @@ elif [[ -z "$BUILDXL_BIN" ]]; then
getLkg
fi
# A casing related PR polluted the cache, so let's force a salt. This could be removed after the poisoned content gets evicted.
# Forcing a salt here to avoid problems faced in Linux validation pipeline related to cache.
# This is related to Bug 2104538 where the cache may or may not be setting the execute bit for some executables.
if [[ $arg_UserProvidedBxlArguments != *"/p:BUILDXL_FINGERPRINT_SALT"* ]]; then
arg_Positional+=("/p:BUILDXL_FINGERPRINT_SALT=casingPR")
arg_Positional+=("/p:BUILDXL_FINGERPRINT_SALT=forceSaltForLinuxBugFixPR")
fi
compileWithBxl ${arg_Positional[@]} ${arg_UserProvidedBxlArguments[@]}