[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: r243945 - [UB] Fix two cases of UB in copy/pasted code from SmallVector.
From:       Hans Wennborg via cfe-commits <cfe-commits () lists ! llvm ! org>
Date:       2015-08-06 15:54:38
Message-ID: CAB8jPhdYnMj5_JA534sHp4QXg6V+m9EA+Q4VHr7fUw4PCZEdOQ () mail ! gmail ! com
[Download RAW message or body]

Great, all merged in r244223.

Thanks,
Hans

On Wed, Aug 5, 2015 at 2:58 PM, Richard Smith <richard@metafoo.co.uk> wrote:
> These all look completely safe; I'm happy with 243945-243950 going to the
> branch.
> 
> On Wed, Aug 5, 2015 at 11:56 AM, Hans Wennborg <hans@chromium.org> wrote:
> > 
> > Richard, Chandler asked me about merging this and the other memcpy-ub
> > patches to 3.7. I'd like to hear what you think.
> > 
> > On the one hand, this doesn't fix a regression from previous releases
> > and the issue it addresses is probably fairly benign at the moment. On
> > the other hand, the patches do fix undefined behaviour and look pretty
> > straight-forward. What do you think?
> > 
> > Thanks,
> > Hans
> > 
> > On Mon, Aug 3, 2015 at 8:52 PM, Chandler Carruth <chandlerc@gmail.com>
> > wrote:
> > > Author: chandlerc
> > > Date: Mon Aug  3 22:52:52 2015
> > > New Revision: 243945
> > > 
> > > URL: http://llvm.org/viewvc/llvm-project?rev=243945&view=rev
> > > Log:
> > > [UB] Fix two cases of UB in copy/pasted code from SmallVector.
> > > 
> > > We should really stop copying and pasting code around. =/
> > > 
> > > Found by UBSan.
> > > 
> > > Modified:
> > > cfe/trunk/include/clang/AST/ASTVector.h
> > > cfe/trunk/include/clang/Analysis/Support/BumpVector.h
> > > 
> > > Modified: cfe/trunk/include/clang/AST/ASTVector.h
> > > URL:
> > > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTVector.h?rev=243945&r1=243944&r2=243945&view=diff
> > >  
> > > ==============================================================================
> > > --- cfe/trunk/include/clang/AST/ASTVector.h (original)
> > > +++ cfe/trunk/include/clang/AST/ASTVector.h Mon Aug  3 22:52:52 2015
> > > @@ -384,14 +384,15 @@ void ASTVector<T>::grow(const ASTContext
> > > T *NewElts = new (C, llvm::alignOf<T>()) T[NewCapacity];
> > > 
> > > // Copy the elements over.
> > > -  if (std::is_class<T>::value) {
> > > -    std::uninitialized_copy(Begin, End, NewElts);
> > > -    // Destroy the original elements.
> > > -    destroy_range(Begin, End);
> > > -  }
> > > -  else {
> > > -    // Use memcpy for PODs (std::uninitialized_copy optimizes to
> > > memmove).
> > > -    memcpy(NewElts, Begin, CurSize * sizeof(T));
> > > +  if (Begin != End) {
> > > +    if (std::is_class<T>::value) {
> > > +      std::uninitialized_copy(Begin, End, NewElts);
> > > +      // Destroy the original elements.
> > > +      destroy_range(Begin, End);
> > > +    } else {
> > > +      // Use memcpy for PODs (std::uninitialized_copy optimizes to
> > > memmove).
> > > +      memcpy(NewElts, Begin, CurSize * sizeof(T));
> > > +    }
> > > }
> > > 
> > > // ASTContext never frees any memory.
> > > 
> > > Modified: cfe/trunk/include/clang/Analysis/Support/BumpVector.h
> > > URL:
> > > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Support/BumpVector.h?rev=243945&r1=243944&r2=243945&view=diff
> > >  
> > > ==============================================================================
> > > --- cfe/trunk/include/clang/Analysis/Support/BumpVector.h (original)
> > > +++ cfe/trunk/include/clang/Analysis/Support/BumpVector.h Mon Aug  3
> > > 22:52:52 2015
> > > @@ -223,14 +223,15 @@ void BumpVector<T>::grow(BumpVectorConte
> > > T *NewElts = C.getAllocator().template Allocate<T>(NewCapacity);
> > > 
> > > // Copy the elements over.
> > > -  if (std::is_class<T>::value) {
> > > -    std::uninitialized_copy(Begin, End, NewElts);
> > > -    // Destroy the original elements.
> > > -    destroy_range(Begin, End);
> > > -  }
> > > -  else {
> > > -    // Use memcpy for PODs (std::uninitialized_copy optimizes to
> > > memmove).
> > > -    memcpy(NewElts, Begin, CurSize * sizeof(T));
> > > +  if (Begin != End) {
> > > +    if (std::is_class<T>::value) {
> > > +      std::uninitialized_copy(Begin, End, NewElts);
> > > +      // Destroy the original elements.
> > > +      destroy_range(Begin, End);
> > > +    } else {
> > > +      // Use memcpy for PODs (std::uninitialized_copy optimizes to
> > > memmove).
> > > +      memcpy(NewElts, Begin, CurSize * sizeof(T));
> > > +    }
> > > }
> 
> 
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]