Skip to content

Commit

Permalink
Fix use-after-free in some helper methods
Browse files Browse the repository at this point in the history
llvm::SmallString's str() method returns an llvm::StringRef, which does
not copy the string data. When getShortDescription/getVerboseDescription
return, the SmallString is deallocated and the underlying memory freed,
causing the returned StringRef to be pointing to deallocated memory.

Remove these functions and require the caller to provide their own
string data instance, which the Command can then populate with data.
  • Loading branch information
jakepetroules authored and dmbryson committed Jun 19, 2018
1 parent 3dd773b commit d89170d
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 16 deletions.
14 changes: 0 additions & 14 deletions include/llbuild/BuildSystem/BuildDescription.h
Original file line number Diff line number Diff line change
Expand Up @@ -181,24 +181,10 @@ class Command {

/// Get a short description of the command, for use in status reporting.
virtual void getShortDescription(SmallVectorImpl<char> &result) = 0;

/// Get a short description of the command, for use in status reporting.
llvm::StringRef getShortDescription() {
llvm::SmallString<64> description;
getShortDescription(description);
return description.str();
}

/// Get a verbose description of the command, for use in status reporting.
virtual void getVerboseDescription(SmallVectorImpl<char> &result) = 0;

/// Get a verbose description of the command, for use in status reporting.
llvm::StringRef getVerboseDescription() {
llvm::SmallString<64> description;
getVerboseDescription(description);
return description.str();
}

/// @}

/// @name File Loading
Expand Down
10 changes: 8 additions & 2 deletions lib/BuildSystem/LaneBasedExecutionQueue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,10 @@ class LaneBasedExecutionQueue : public BuildExecutionQueue {
LaneBasedExecutionQueueJobContext context{ laneNumber, job };
{
TracingExecutionQueueDepth(readyJobsCount);
TracingExecutionQueueJob t(context.laneNumber, job.getForCommand()->getShortDescription());

llvm::SmallString<64> description;
job.getForCommand()->getShortDescription(description);
TracingExecutionQueueJob t(context.laneNumber, description.str());

getDelegate().commandJobStarted(job.getForCommand());
job.execute(reinterpret_cast<QueueJobContext*>(&context));
Expand Down Expand Up @@ -309,7 +312,10 @@ class LaneBasedExecutionQueue : public BuildExecutionQueue {
bool canSafelyInterrupt) override {
LaneBasedExecutionQueueJobContext& context =
*reinterpret_cast<LaneBasedExecutionQueueJobContext*>(opaqueContext);
TracingExecutionQueueSubprocess subprocessInterval(context.laneNumber, context.job.getForCommand()->getShortDescription());

llvm::SmallString<64> description;
context.job.getForCommand()->getShortDescription(description);
TracingExecutionQueueSubprocess subprocessInterval(context.laneNumber, description.str());

{
std::unique_lock<std::mutex> lock(readyJobsMutex);
Expand Down

0 comments on commit d89170d

Please sign in to comment.