[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: Richard Smith <richard () metafoo ! co ! uk>
Date: 2015-08-05 21:58:04
Message-ID: CAOfiQqnMQSJMwO8FPbyrmazTuO9DWhH4XHWF6hL7voYfXtMu8Q () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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));
> > + }
> > }
>
[Attachment #5 (text/html)]
<div dir="ltr">These all look completely safe; I'm happy with <span \
style="font-size:12.8000001907349px">243945-243950 going to the \
branch.</span></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug \
5, 2015 at 11:56 AM, Hans Wennborg <span dir="ltr"><<a \
href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> \
wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px \
#ccc solid;padding-left:1ex">Richard, Chandler asked me about merging this and the \
other memcpy-ub<br> patches to 3.7. I'd like to hear what you think.<br>
<br>
On the one hand, this doesn't fix a regression from previous releases<br>
and the issue it addresses is probably fairly benign at the moment. On<br>
the other hand, the patches do fix undefined behaviour and look pretty<br>
straight-forward. What do you think?<br>
<br>
Thanks,<br>
Hans<br>
<div class="HOEnZb"><div class="h5"><br>
On Mon, Aug 3, 2015 at 8:52 PM, Chandler Carruth <<a \
href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>> wrote:<br> > Author: \
chandlerc<br> > Date: Mon Aug 3 22:52:52 2015<br>
> New Revision: 243945<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=243945&view=rev" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=243945&view=rev</a><br>
> Log:<br>
> [UB] Fix two cases of UB in copy/pasted code from SmallVector.<br>
><br>
> We should really stop copying and pasting code around. =/<br>
><br>
> Found by UBSan.<br>
><br>
> Modified:<br>
> cfe/trunk/include/clang/AST/ASTVector.h<br>
> cfe/trunk/include/clang/Analysis/Support/BumpVector.h<br>
><br>
> Modified: cfe/trunk/include/clang/AST/ASTVector.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTVector.h?rev=243945&r1=243944&r2=243945&view=diff" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include \
/clang/AST/ASTVector.h?rev=243945&r1=243944&r2=243945&view=diff</a><br> \
> ==============================================================================<br>
> --- cfe/trunk/include/clang/AST/ASTVector.h (original)<br>
> +++ cfe/trunk/include/clang/AST/ASTVector.h Mon Aug 3 22:52:52 2015<br>
> @@ -384,14 +384,15 @@ void ASTVector<T>::grow(const ASTContext<br>
> T *NewElts = new (C, llvm::alignOf<T>()) T[NewCapacity];<br>
><br>
> // Copy the elements over.<br>
> - if (std::is_class<T>::value) {<br>
> - std::uninitialized_copy(Begin, End, NewElts);<br>
> - // Destroy the original elements.<br>
> - destroy_range(Begin, End);<br>
> - }<br>
> - else {<br>
> - // Use memcpy for PODs (std::uninitialized_copy optimizes to \
memmove).<br> > - memcpy(NewElts, Begin, CurSize * sizeof(T));<br>
> + if (Begin != End) {<br>
> + if (std::is_class<T>::value) {<br>
> + std::uninitialized_copy(Begin, End, NewElts);<br>
> + // Destroy the original elements.<br>
> + destroy_range(Begin, End);<br>
> + } else {<br>
> + // Use memcpy for PODs (std::uninitialized_copy optimizes to \
memmove).<br> > + memcpy(NewElts, Begin, CurSize * sizeof(T));<br>
> + }<br>
> }<br>
><br>
> // ASTContext never frees any memory.<br>
><br>
> Modified: cfe/trunk/include/clang/Analysis/Support/BumpVector.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Support/BumpVector.h?rev=243945&r1=243944&r2=243945&view=diff" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include \
/clang/Analysis/Support/BumpVector.h?rev=243945&r1=243944&r2=243945&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/include/clang/Analysis/Support/BumpVector.h (original)<br>
> +++ cfe/trunk/include/clang/Analysis/Support/BumpVector.h Mon Aug 3 22:52:52 \
2015<br> > @@ -223,14 +223,15 @@ void \
BumpVector<T>::grow(BumpVectorConte<br> > T *NewElts = \
C.getAllocator().template Allocate<T>(NewCapacity);<br> ><br>
> // Copy the elements over.<br>
> - if (std::is_class<T>::value) {<br>
> - std::uninitialized_copy(Begin, End, NewElts);<br>
> - // Destroy the original elements.<br>
> - destroy_range(Begin, End);<br>
> - }<br>
> - else {<br>
> - // Use memcpy for PODs (std::uninitialized_copy optimizes to \
memmove).<br> > - memcpy(NewElts, Begin, CurSize * sizeof(T));<br>
> + if (Begin != End) {<br>
> + if (std::is_class<T>::value) {<br>
> + std::uninitialized_copy(Begin, End, NewElts);<br>
> + // Destroy the original elements.<br>
> + destroy_range(Begin, End);<br>
> + } else {<br>
> + // Use memcpy for PODs (std::uninitialized_copy optimizes to \
memmove).<br> > + memcpy(NewElts, Begin, CurSize * sizeof(T));<br>
> + }<br>
> }<br>
</div></div></blockquote></div><br></div>
[Attachment #6 (text/plain)]
_______________________________________________
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]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic