[llvm] r197178 - Fix a use-after-free error in GlobalOpt CleanupConstantGlobalUsers

[Hal Finkel]

Chris,

This seems like a low-risk fix, and it is pretty easy to write C code which triggers this (I've only ever seen this either assert or cause a segfault, but I can imagine it miscompiling something with NDEBUG). How do you feel about putting this into the release branch?

 -Hal

----- Original Message -----
> From: "Hal Finkel" <hfinkel at anl.gov>
> To: llvm-commits at cs.uiuc.edu
> Sent: Thursday, December 12, 2013 2:45:24 PM
> Subject: [llvm] r197178 - Fix a use-after-free error in GlobalOpt	CleanupConstantGlobalUsers
> 
> Author: hfinkel
> Date: Thu Dec 12 14:45:24 2013
> New Revision: 197178
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=197178&view=rev
> Log:
> Fix a use-after-free error in GlobalOpt CleanupConstantGlobalUsers
> 
> GlobalOpt's CleanupConstantGlobalUsers function uses a worklist array
> to manage
> constant users to be visited. The pointers in this array need to be
> weak
> handles because when we delete a constant array, we may also be
> holding a
> pointer to one of its elements (or an element of one of its elements
> if we're
> dealing with an array of arrays) in the worklist.
> 
> Fixes PR17347.
> 
> Added:
>     llvm/trunk/test/Transforms/GlobalOpt/array-elem-refs.ll
> Modified:
>     llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp
> 
> Modified: llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp?rev=197178&r1=197177&r2=197178&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp (original)
> +++ llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp Thu Dec 12 14:45:24
> 2013
> @@ -37,6 +37,7 @@
>  #include "llvm/Support/GetElementPtrTypeIterator.h"
>  #include "llvm/Support/MathExtras.h"
>  #include "llvm/Support/raw_ostream.h"
> +#include "llvm/Support/ValueHandle.h"
>  #include "llvm/Target/TargetLibraryInfo.h"
>  #include "llvm/Transforms/Utils/GlobalStatus.h"
>  #include "llvm/Transforms/Utils/ModuleUtils.h"
> @@ -267,9 +268,17 @@ static bool CleanupPointerRootUsers(Glob
>  static bool CleanupConstantGlobalUsers(Value *V, Constant *Init,
>                                         DataLayout *TD,
>                                         TargetLibraryInfo *TLI) {
>    bool Changed = false;
> -  SmallVector<User*, 8> WorkList(V->use_begin(), V->use_end());
> +  // Note that we need to use a weak value handle for the worklist
> items. When
> +  // we delete a constant array, we may also be holding pointer to
> one of its
> +  // elements (or an element of one of its elements if we're dealing
> with an
> +  // array of arrays) in the worklist.
> +  SmallVector<WeakVH, 8> WorkList(V->use_begin(), V->use_end());
>    while (!WorkList.empty()) {
> -    User *U = WorkList.pop_back_val();
> +    Value *UV = WorkList.pop_back_val();
> +    if (!UV)
> +      continue;
> +
> +    User *U = cast<User>(UV);
>  
>      if (LoadInst *LI = dyn_cast<LoadInst>(U)) {
>        if (Init) {
> 
> Added: llvm/trunk/test/Transforms/GlobalOpt/array-elem-refs.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GlobalOpt/array-elem-refs.ll?rev=197178&view=auto
> ==============================================================================
> --- llvm/trunk/test/Transforms/GlobalOpt/array-elem-refs.ll (added)
> +++ llvm/trunk/test/Transforms/GlobalOpt/array-elem-refs.ll Thu Dec
> 12 14:45:24 2013
> @@ -0,0 +1,32 @@
> +; RUN: opt < %s -S -globalopt | FileCheck %s
> +target datalayout =
> "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
> +target triple = "x86_64-unknown-linux-gnu"
> +
> +%struct.S = type { i8, i8 }
> +
> + at c = internal global i8** bitcast (i8* getelementptr (i8* bitcast
> ([8 x i8*]* @b to i8*), i64 48) to i8**), align 8
> + at b = internal global [8 x i8*] [i8* null, i8* null, i8* null, i8*
> null, i8* null, i8* null, i8* getelementptr inbounds (%struct.S* @a,
> i32 0, i32 0), i8* getelementptr (i8* getelementptr inbounds
> (%struct.S* @a, i32 0, i32 0), i64 1)], align 16
> + at a = internal global %struct.S zeroinitializer, align 1
> +
> +; Function Attrs: nounwind uwtable
> +define signext i8 @foo() #0 {
> +entry:
> +  %0 = load i8*** @c, align 8
> +  %1 = load i8** %0, align 8
> +  %2 = load i8* %1, align 1
> +  ret i8 %2
> +
> +; CHECK-LABEL: @foo
> +; CHECK: ret i8 0
> +}
> +
> +; Function Attrs: nounwind uwtable
> +define i32 @main() #0 {
> +entry:
> +  %retval = alloca i32, align 4
> +  store i32 0, i32* %retval
> +  ret i32 0
> +}
> +
> +attributes #0 = { nounwind uwtable }
> +
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory