[polly] 6983741 - [Polly] Fix use-after-free.

Michael Kruse via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 26 11:17:34 PDT 2020


Thanks for the suggestions. Implemented
in c971b53b22a5cd43b54bf4773fe3c59ea1b805fb
and 6538fff37245921a0983d94c08af7e6cc120b3a9.

Michael

Am Mo., 24. Aug. 2020 um 12:42 Uhr schrieb David Blaikie <dblaikie at gmail.com
>:

>
>
> On Sat, Aug 22, 2020 at 8:22 AM Michael Kruse via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>>
>> Author: Michael Kruse
>> Date: 2020-08-22T10:10:49-05:00
>> New Revision: 6983741eaa84e76802feca9145e39293cc6d15b4
>>
>> URL:
>> https://github.com/llvm/llvm-project/commit/6983741eaa84e76802feca9145e39293cc6d15b4
>> DIFF:
>> https://github.com/llvm/llvm-project/commit/6983741eaa84e76802feca9145e39293cc6d15b4.diff
>>
>> LOG: [Polly] Fix use-after-free.
>>
>> VirtualUse of type UseKind::Inter expects the definition of a
>> llvm::Value to be represented in another statement. In the bug report
>> that statement has been removed due to its domain being empty.
>> Scop::InstStmtMap for the llvm::Value's defintion still pointed to the
>> removed statement, which resulted in the use-after-free.
>>
>> The defintion statement was removed by Simplify because it was
>> considered to not be reachable by other uses; trivially because it is
>> never executed due to its empty domain. However, no such thing happend
>> to the using statement using the value altough its domain is also empty.
>>
>> Fix by always removing statements with empty domains in Simplify since
>> these are not properly analyzable. A UseKind::Inter should always have a
>> statement with its defintion due to LLVM's SSA form.
>> Scop::removeStmtNotInDomainMap() also removes statements with empty
>> domains but does so without considering the context as used by
>> Simplify's analyzes.
>>
>> In another angle, InstStmtMap pointing to removed statements should not
>> happen either and ForwardOpTree would have bailed out if the llvm::Value
>> definition was not represented by a statement. This will be corrected in
>> a followup-commit.
>>
>> This fixes llvm.org/PR47098
>>
>> Added:
>>     polly/test/Simplify/func-b320a7.ll
>>
>> Modified:
>>     polly/include/polly/ScopInfo.h
>>     polly/lib/Transform/Simplify.cpp
>>
>> Removed:
>>
>>
>>
>>
>> ################################################################################
>> diff  --git a/polly/include/polly/ScopInfo.h
>> b/polly/include/polly/ScopInfo.h
>> index 7d99a38086d9..b6fcddc6379a 100644
>> --- a/polly/include/polly/ScopInfo.h
>> +++ b/polly/include/polly/ScopInfo.h
>> @@ -1949,18 +1949,6 @@ class Scop {
>>    void addScopStmt(Region *R, StringRef Name, Loop *SurroundingLoop,
>>                     std::vector<Instruction *> EntryBlockInstructions);
>>
>> -  /// Remove statements from the list of scop statements.
>> -  ///
>> -  /// @param ShouldDelete  A function that returns true if the statement
>> passed
>> -  ///                      to it should be deleted.
>> -  /// @param AfterHoisting If true, also remove from data access lists.
>> -  ///                      These lists are filled during
>> -  ///                      ScopBuilder::buildAccessRelations. Therefore,
>> if this
>> -  ///                      method is called before buildAccessRelations,
>> false
>> -  ///                      must be passed.
>> -  void removeStmts(std::function<bool(ScopStmt &)> ShouldDelete,
>> -                   bool AfterHoisting = true);
>> -
>>    /// Removes @p Stmt from the StmtMap.
>>    void removeFromStmtMap(ScopStmt &Stmt);
>>
>> @@ -2321,6 +2309,19 @@ class Scop {
>>      MinMaxAliasGroups.back().first = MinMaxAccessesReadWrite;
>>      MinMaxAliasGroups.back().second = MinMaxAccessesReadOnly;
>>    }
>> +
>> +  /// Remove statements from the list of scop statements.
>> +  ///
>> +  /// @param ShouldDelete  A function that returns true if the statement
>> passed
>> +  ///                      to it should be deleted.
>> +  /// @param AfterHoisting If true, also remove from data access lists.
>> +  ///                      These lists are filled during
>> +  ///                      ScopBuilder::buildAccessRelations. Therefore,
>> if this
>> +  ///                      method is called before buildAccessRelations,
>> false
>> +  ///                      must be passed.
>> +  void removeStmts(std::function<bool(ScopStmt &)> ShouldDelete,
>> +                   bool AfterHoisting = true);
>>
>
> Unrelated to this patch (or a pre-existing condition at least) but
> assuming removeStmts doesn't capture "ShouldDelete" (only uses it within
> the scope of the function call) - it could use llvm::function_ref instead,
> which can avoid some overhead.
>
>
>> +
>>    /// Get an isl string representing the context.
>>    std::string getContextStr() const;
>>
>>
>> diff  --git a/polly/lib/Transform/Simplify.cpp
>> b/polly/lib/Transform/Simplify.cpp
>> index ec9df1846ec5..f3b8bf83efe5 100644
>> --- a/polly/lib/Transform/Simplify.cpp
>> +++ b/polly/lib/Transform/Simplify.cpp
>> @@ -41,6 +41,8 @@ static int const SimplifyMaxDisjuncts = 4;
>>  TWO_STATISTICS(ScopsProcessed, "Number of SCoPs processed");
>>  TWO_STATISTICS(ScopsModified, "Number of SCoPs simplified");
>>
>> +TWO_STATISTICS(TotalEmptyDomainsRemoved,
>> +               "Number of statement with empty domains removed in any
>> SCoP");
>>  TWO_STATISTICS(TotalOverwritesRemoved, "Number of removed overwritten
>> writes");
>>  TWO_STATISTICS(TotalWritesCoalesced, "Number of writes coalesced with
>> another");
>>  TWO_STATISTICS(TotalRedundantWritesRemoved,
>> @@ -124,6 +126,9 @@ class Simplify : public ScopPass {
>>    /// The last/current SCoP that is/has been processed.
>>    Scop *S;
>>
>> +  /// Number of statements with empty domains removed from the SCoP.
>> +  int EmptyDomainsRemoved = 0;
>> +
>>    /// Number of writes that are overwritten anyway.
>>    int OverwritesRemoved = 0;
>>
>> @@ -147,10 +152,36 @@ class Simplify : public ScopPass {
>>
>>    /// Return whether at least one simplification has been applied.
>>    bool isModified() const {
>> -    return OverwritesRemoved > 0 || WritesCoalesced > 0 ||
>> -           RedundantWritesRemoved > 0 || EmptyPartialAccessesRemoved > 0
>> ||
>> -           DeadAccessesRemoved > 0 || DeadInstructionsRemoved > 0 ||
>> -           StmtsRemoved > 0;
>> +    return EmptyDomainsRemoved > 0 || OverwritesRemoved > 0 ||
>> +           WritesCoalesced > 0 || RedundantWritesRemoved > 0 ||
>> +           EmptyPartialAccessesRemoved > 0 || DeadAccessesRemoved > 0 ||
>> +           DeadInstructionsRemoved > 0 || StmtsRemoved > 0;
>> +  }
>> +
>> +  /// Remove statements that are never executed due to their domains
>> being
>> +  /// empty.
>> +  ///
>> +  /// In contrast to Scop::simplifySCoP, this removes based on the SCoP's
>> +  /// effective domain, i.e. including the SCoP's context as used by
>> some other
>> +  /// simplification methods in this pass. This is necessary because the
>> +  /// analysis on empty domains is unreliable, e.g. remove a scalar value
>> +  /// definition MemoryAccesses, but not its use.
>> +  void removeEmptyDomainStmts() {
>> +    size_t NumStmtsBefore = S->getSize();
>> +
>> +    auto ShouldDelete = [](ScopStmt &Stmt) -> bool {
>> +      auto EffectiveDomain =
>> +
>> Stmt.getDomain().intersect_params(Stmt.getParent()->getContext());
>> +      return EffectiveDomain.is_empty();
>> +    };
>> +    S->removeStmts(ShouldDelete);
>>
>
> FWIW, I'd probably put the lambda directly inside the "removeStmts" call -
> named lambdas are always a bit more "interesting" (where else is it called,
> how does it capture, etc) than ones that are used only within the scope of
> a single expression.
>
>
>> +
>> +    assert(NumStmtsBefore >= S->getSize());
>> +    EmptyDomainsRemoved = NumStmtsBefore - S->getSize();
>> +    LLVM_DEBUG(dbgs() << "Removed " << EmptyDomainsRemoved << " (of "
>> +                      << NumStmtsBefore
>> +                      << ") statements with empty domains \n");
>> +    TotalEmptyDomainsRemoved[CallNo] += EmptyDomainsRemoved;
>>    }
>>
>>    /// Remove writes that are overwritten unconditionally later in the
>> same
>> @@ -587,6 +618,8 @@ class Simplify : public ScopPass {
>>    /// Print simplification statistics to @p OS.
>>    void printStatistics(llvm::raw_ostream &OS, int Indent = 0) const {
>>      OS.indent(Indent) << "Statistics {\n";
>> +    OS.indent(Indent + 4) << "Empty domains removed: " <<
>> EmptyDomainsRemoved
>> +                          << '\n';
>>      OS.indent(Indent + 4) << "Overwrites removed: " << OverwritesRemoved
>>                            << '\n';
>>      OS.indent(Indent + 4) << "Partial writes coalesced: " <<
>> WritesCoalesced
>> @@ -633,6 +666,9 @@ class Simplify : public ScopPass {
>>      this->S = &S;
>>      ScopsProcessed[CallNo]++;
>>
>> +    LLVM_DEBUG(dbgs() << "Removing statements that are never
>> executed...\n");
>> +    removeEmptyDomainStmts();
>> +
>>      LLVM_DEBUG(dbgs() << "Removing partial writes that never
>> happen...\n");
>>      removeEmptyPartialAccesses();
>>
>> @@ -683,6 +719,7 @@ class Simplify : public ScopPass {
>>    virtual void releaseMemory() override {
>>      S = nullptr;
>>
>> +    EmptyDomainsRemoved = 0;
>>      OverwritesRemoved = 0;
>>      WritesCoalesced = 0;
>>      RedundantWritesRemoved = 0;
>>
>> diff  --git a/polly/test/Simplify/func-b320a7.ll
>> b/polly/test/Simplify/func-b320a7.ll
>> new file mode 100644
>> index 000000000000..22f6a5ac98f1
>> --- /dev/null
>> +++ b/polly/test/Simplify/func-b320a7.ll
>> @@ -0,0 +1,82 @@
>> +; RUN: opt %loadPolly -polly-simplify -polly-optree -analyze < %s |
>> FileCheck %s -match-full-lines
>> +
>> +; llvm.org/PR47098
>> +; Use-after-free by reference to Stmt remaining in InstStmtMap after
>> removing it has been removed by Scop::simplifyScop.
>> +; Removal happened in -polly-simplify, Reference was using in
>> -polly-optree.
>> +; Check that Simplify removes the definition of %0 as well of its use.
>> +
>> +target datalayout =
>> "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
>> +target triple = "x86_64-pc-windows-msvc19.26.28806"
>> +
>> +@"?var_27@@3JA" = external dso_local local_unnamed_addr global i32,
>> align 4
>> +@"?var_28@@3HA" = external dso_local local_unnamed_addr global i32,
>> align 4
>> +
>> +; Function Attrs: nofree norecurse nounwind uwtable
>> +define dso_local void @"?test@@YAXHEQEAY3M at 1BI@BJ at H@Z"(i32 %a, i8 %b,
>> [12 x [2 x [24 x [25 x i32]]]]* nocapture readonly %c) local_unnamed_addr {
>> +entry:
>> +  br label %entry.split
>> +
>> +entry.split:                                      ; preds = %entry
>> +  %sext = shl i32 %a, 16
>> +  %conv4 = ashr exact i32 %sext, 16
>> +  %sub = add nsw i32 %conv4, -25941
>> +  %cmp535 = icmp sgt i32 %sext, 1700069376
>> +  br i1 %cmp535, label %for.cond8.preheader.lr.ph, label
>> %for.cond.cleanup.critedge
>> +
>> +for.cond8.preheader.lr.ph:                        ; preds = %entry.split
>> +  %conv10 = zext i8 %b to i64
>> +  %sub11 = add nsw i64 %conv10, -129
>> +  %cmp1232.not = icmp eq i64 %sub11, 0
>> +  br i1 %cmp1232.not, label %for.cond8.preheader, label %
>> for.cond8.preheader.us
>> +
>> +for.cond8.preheader.us:                           ; preds = %
>> for.cond8.preheader.lr.ph, %for.cond8.for.cond.cleanup13_crit_edge.us
>> +  %e.036.us = phi i16 [ %add.us, %
>> for.cond8.for.cond.cleanup13_crit_edge.us ], [ 0, %
>> for.cond8.preheader.lr.ph ]
>> +  %idxprom.us = sext i16 %e.036.us to i64
>> +  br label %for.body14.us
>> +
>> +for.body14.us:                                    ; preds = %
>> for.cond8.preheader.us, %for.body14.us
>> +  %indvars.iv = phi i64 [ 0, %for.cond8.preheader.us ], [
>> %indvars.iv.next, %for.body14.us ]
>> +  %arrayidx19.us = getelementptr inbounds [12 x [2 x [24 x [25 x
>> i32]]]], [12 x [2 x [24 x [25 x i32]]]]* %c, i64 6, i64 2, i64 1, i64 %
>> idxprom.us, i64 %indvars.iv
>> +  %0 = load i32, i32* %arrayidx19.us, align 4, !tbaa !3
>> +  store i32 0, i32* @"?var_28@@3HA", align 4, !tbaa !3
>> +  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
>> +  %exitcond.not = icmp eq i64 %indvars.iv.next, %sub11
>> +  br i1 %exitcond.not, label %for.cond8.for.cond.cleanup13_crit_edge.us,
>> label %for.body14.us
>> +
>> +for.cond8.for.cond.cleanup13_crit_edge.us:        ; preds = %
>> for.body14.us
>> +  %add.us = add i16 %e.036.us, 4
>> +  %conv2.us = sext i16 %add.us to i32
>> +  %cmp5.us = icmp sgt i32 %sub, %conv2.us
>> +  br i1 %cmp5.us, label %for.cond8.preheader.us, label
>> %for.cond.cleanup.critedge.loopexit38
>> +
>> +for.cond.cleanup.critedge.loopexit38:             ; preds = %
>> for.cond8.for.cond.cleanup13_crit_edge.us
>> +  store i32 %0, i32* @"?var_27@@3JA", align 4, !tbaa !7
>> +  br label %for.cond.cleanup.critedge
>> +
>> +for.cond.cleanup.critedge:                        ; preds =
>> %for.cond8.preheader, %for.cond.cleanup.critedge.loopexit38, %entry.split
>> +  ret void
>> +
>> +for.cond8.preheader:                              ; preds = %
>> for.cond8.preheader.lr.ph, %for.cond8.preheader
>> +  %e.036 = phi i16 [ %add, %for.cond8.preheader ], [ 0, %
>> for.cond8.preheader.lr.ph ]
>> +  %add = add i16 %e.036, 4
>> +  %conv2 = sext i16 %add to i32
>> +  %cmp5 = icmp sgt i32 %sub, %conv2
>> +  br i1 %cmp5, label %for.cond8.preheader, label
>> %for.cond.cleanup.critedge
>> +}
>> +
>> +!3 = !{!4, !4, i64 0}
>> +!4 = !{!"int", !5, i64 0}
>> +!5 = !{!"omnipotent char", !6, i64 0}
>> +!6 = !{!"Simple C++ TBAA"}
>> +!7 = !{!8, !8, i64 0}
>> +!8 = !{!"long", !5, i64 0}
>> +
>> +
>> +; CHECK: Statistics {
>> +; CHECK:     Empty domains removed: 2
>> +; CHECK: }
>> +
>> +; CHECK: After accesses {
>> +; CHECK-NOT: Stmt_for_body14_us_a
>> +; CHECK-NOT: Stmt_for_body14_us
>> +; CHECK: }
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>

-- 
Tardyzentrismus verboten!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200826/33919ae7/attachment-0001.html>


More information about the llvm-commits mailing list