From cfe-commits Wed Aug 05 21:58:04 2015 From: Richard Smith Date: Wed, 05 Aug 2015 21:58:04 +0000 To: cfe-commits Subject: Re: r243945 - [UB] Fix two cases of UB in copy/pasted code from SmallVector. Message-Id: X-MARC-Message: https://marc.info/?l=cfe-commits&m=143881189030630 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============6405433461207325401==" --===============6405433461207325401== Content-Type: multipart/alternative; boundary=001a11409ade4c9a0f051c977f80 --001a11409ade4c9a0f051c977f80 Content-Type: text/plain; charset=UTF-8 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 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 > 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::grow(const ASTContext > > T *NewElts = new (C, llvm::alignOf()) T[NewCapacity]; > > > > // Copy the elements over. > > - if (std::is_class::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::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::grow(BumpVectorConte > > T *NewElts = C.getAllocator().template Allocate(NewCapacity); > > > > // Copy the elements over. > > - if (std::is_class::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::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)); > > + } > > } > --001a11409ade4c9a0f051c977f80 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
These all look completely safe; I'm happy with=C2=A0243945-243950 going to the branc= h.

On= Wed, Aug 5, 2015 at 11:56 AM, Hans Wennborg <hans@chromium.org> wrote:
Richard, Chandler asked me a= bout 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=C2=A0 3 22:52:52 2015
> New Revision: 243945
>
> URL: http://llvm.org/viewvc/llvm= -project?rev=3D243945&view=3Drev
> Log:
> [UB] Fix two cases of UB in copy/pasted code from SmallVector.
>
> We should really stop copying and pasting code around. =3D/
>
> Found by UBSan.
>
> Modified:
>=C2=A0 =C2=A0 =C2=A0cfe/trunk/include/clang/AST/ASTVector.h
>=C2=A0 =C2=A0 =C2=A0cfe/trunk/include/clang/Analysis/Support/BumpVector= .h
>
> Modified: cfe/trunk/include/clang/AST/ASTVector.h
> URL: http://llvm.org/viewvc/llvm-p= roject/cfe/trunk/include/clang/AST/ASTVector.h?rev=3D243945&r1=3D243944= &r2=3D243945&view=3Ddiff
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D
> --- cfe/trunk/include/clang/AST/ASTVector.h (original)
> +++ cfe/trunk/include/clang/AST/ASTVector.h Mon Aug=C2=A0 3 22:52:52 2= 015
> @@ -384,14 +384,15 @@ void ASTVector<T>::grow(const ASTContext >=C2=A0 =C2=A0 T *NewElts =3D new (C, llvm::alignOf<T>()) T[NewCap= acity];
>
>=C2=A0 =C2=A0 // Copy the elements over.
> -=C2=A0 if (std::is_class<T>::value) {
> -=C2=A0 =C2=A0 std::uninitialized_copy(Begin, End, NewElts);
> -=C2=A0 =C2=A0 // Destroy the original elements.
> -=C2=A0 =C2=A0 destroy_range(Begin, End);
> -=C2=A0 }
> -=C2=A0 else {
> -=C2=A0 =C2=A0 // Use memcpy for PODs (std::uninitialized_copy optimiz= es to memmove).
> -=C2=A0 =C2=A0 memcpy(NewElts, Begin, CurSize * sizeof(T));
> +=C2=A0 if (Begin !=3D End) {
> +=C2=A0 =C2=A0 if (std::is_class<T>::value) {
> +=C2=A0 =C2=A0 =C2=A0 std::uninitialized_copy(Begin, End, NewElts); > +=C2=A0 =C2=A0 =C2=A0 // Destroy the original elements.
> +=C2=A0 =C2=A0 =C2=A0 destroy_range(Begin, End);
> +=C2=A0 =C2=A0 } else {
> +=C2=A0 =C2=A0 =C2=A0 // Use memcpy for PODs (std::uninitialized_copy = optimizes to memmove).
> +=C2=A0 =C2=A0 =C2=A0 memcpy(NewElts, Begin, CurSize * sizeof(T));
> +=C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 }
>
>=C2=A0 =C2=A0 // 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=3D243945&r1=3D243944&r2=3D243945&view=3Ddiff
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D
> --- cfe/trunk/include/clang/Analysis/Support/BumpVector.h (original) > +++ cfe/trunk/include/clang/Analysis/Support/BumpVector.h Mon Aug=C2= =A0 3 22:52:52 2015
> @@ -223,14 +223,15 @@ void BumpVector<T>::grow(BumpVectorConte >=C2=A0 =C2=A0 T *NewElts =3D C.getAllocator().template Allocate<T>= ;(NewCapacity);
>
>=C2=A0 =C2=A0 // Copy the elements over.
> -=C2=A0 if (std::is_class<T>::value) {
> -=C2=A0 =C2=A0 std::uninitialized_copy(Begin, End, NewElts);
> -=C2=A0 =C2=A0 // Destroy the original elements.
> -=C2=A0 =C2=A0 destroy_range(Begin, End);
> -=C2=A0 }
> -=C2=A0 else {
> -=C2=A0 =C2=A0 // Use memcpy for PODs (std::uninitialized_copy optimiz= es to memmove).
> -=C2=A0 =C2=A0 memcpy(NewElts, Begin, CurSize * sizeof(T));
> +=C2=A0 if (Begin !=3D End) {
> +=C2=A0 =C2=A0 if (std::is_class<T>::value) {
> +=C2=A0 =C2=A0 =C2=A0 std::uninitialized_copy(Begin, End, NewElts); > +=C2=A0 =C2=A0 =C2=A0 // Destroy the original elements.
> +=C2=A0 =C2=A0 =C2=A0 destroy_range(Begin, End);
> +=C2=A0 =C2=A0 } else {
> +=C2=A0 =C2=A0 =C2=A0 // Use memcpy for PODs (std::uninitialized_copy = optimizes to memmove).
> +=C2=A0 =C2=A0 =C2=A0 memcpy(NewElts, Begin, CurSize * sizeof(T));
> +=C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 }

--001a11409ade4c9a0f051c977f80-- --===============6405433461207325401== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KY2ZlLWNvbW1p dHMgbWFpbGluZyBsaXN0CmNmZS1jb21taXRzQGxpc3RzLmxsdm0ub3JnCmh0dHA6Ly9saXN0cy5s bHZtLm9yZy9jZ2ktYmluL21haWxtYW4vbGlzdGluZm8vY2ZlLWNvbW1pdHMK --===============6405433461207325401==--