Fix use-after-free in DestroyHoisting #38965

meg-gupta · 2021-08-19T21:48:43Z

No description provided.

OSSA cannot have critical edges. And DestroyHoisting works only on OSSA.

meg-gupta · 2021-08-19T21:48:51Z

@swift-ci test

meg-gupta · 2021-08-19T21:49:05Z

@swift-ci benchmark

meg-gupta · 2021-08-19T22:05:09Z

@swift-ci test

meg-gupta · 2021-08-19T22:05:18Z

@swift-ci benchmark

meg-gupta · 2021-08-19T22:07:52Z

I found a real bug due to unhandled cases with switch_enum_addr. I could not write a test with a real bug with instructions I handle in this PR like witness_method etc, because apply etc are handled correctly. So the tests show examples with dead instructions like witness_method, load_borrow, etc.

swift-ci · 2021-08-19T22:50:35Z

Performance (x86_64): -O

Regression	OLD	NEW	DELTA	RATIO
DictionaryOfAnyHashableStrings_insert	2254	3864	+71.4%	0.58x (?)
Set.isDisjoint.Box25	285	401	+40.7%	0.71x (?)
Set.isDisjoint.Int50	201	241	+19.9%	0.83x (?)

Improvement	OLD	NEW	DELTA	RATIO
FlattenListLoop	3025	2748	-9.2%	1.10x (?)
Calculator	156	143	-8.3%	1.09x (?)
Data.hash.Medium	29	27	-6.9%	1.07x (?)

Code size: -O

Regression	OLD	NEW	DELTA	RATIO
FlattenList.o	4058	4106	+1.2%	0.99x

Improvement	OLD	NEW	DELTA	RATIO
ArraySubscript.o	3140	3108	-1.0%	1.01x

Performance (x86_64): -Osize

Regression	OLD	NEW	DELTA	RATIO
Data.append.Sequence.64kB.Count.RE.I	20	29	+45.0%	0.69x
Data.append.Sequence.64kB.Count.RE	20	29	+45.0%	0.69x
DataAppendSequence	5400	6900	+27.8%	0.78x (?)
Data.append.Sequence.809B.Count.RE.I	54	67	+24.1%	0.81x (?)
Data.append.Sequence.809B.Count.RE	54	66	+22.2%	0.82x (?)
SequenceAlgosRange	1960	2390	+21.9%	0.82x
SequenceAlgosContiguousArray	1710	1930	+12.9%	0.89x (?)
CSVParsing.UTF16	34	37	+8.8%	0.92x (?)

Improvement	OLD	NEW	DELTA	RATIO
Data.init.Sequence.2047B.Count.I	98	52	-46.9%	1.88x
Data.init.Sequence.2049B.Count.I	98	52	-46.9%	1.88x
Data.init.Sequence.64kB.Count.I	58	31	-46.6%	1.87x
Data.init.Sequence.64kB.Count	58	31	-46.6%	1.87x
Data.init.Sequence.809B.Count	84	51	-39.3%	1.65x
Data.init.Sequence.809B.Count.I	84	52	-38.1%	1.62x
Data.init.Sequence.513B.Count.I	93	63	-32.3%	1.48x
Data.init.Sequence.511B.Count.I	93	63	-32.3%	1.48x

Code size: -Osize

Performance (x86_64): -Onone

Regression	OLD	NEW	DELTA	RATIO
ArrayValueProp	2546	2904	+14.1%	0.88x (?)
Ackermann	1049	1186	+13.1%	0.88x (?)
ArrayLiteral2	1501	1672	+11.4%	0.90x (?)
ArrayValueProp4	3342	3633	+8.7%	0.92x (?)
ArrayValueProp3	3382	3676	+8.7%	0.92x (?)
ArrayValueProp2	3715	4027	+8.4%	0.92x (?)
COWArrayGuaranteedParameterOverhead	5000	5400	+8.0%	0.93x (?)

Improvement	OLD	NEW	DELTA	RATIO
ArrayAppendLatin1Substring	33624	30744	-8.6%	1.09x (?)
Data.hash.Medium	39	36	-7.7%	1.08x (?)

Code size: -swiftlibs

Improvement	OLD	NEW	DELTA	RATIO
libswiftStdlibUnittest.dylib	344064	327680	-4.8%	1.05x

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac mini
  Model Identifier: Macmini8,1
  Processor Name: 6-Core Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

swift-ci · 2021-08-19T23:40:10Z

Build failed
Swift Test Linux Platform
Git Sha - 131e4cd874ad5ca8bc19fa5d1164ad7516cc43b4

swift-ci · 2021-08-20T02:09:50Z

Build failed
Swift Test OS X Platform
Git Sha - 131e4cd874ad5ca8bc19fa5d1164ad7516cc43b4

eeckstein

Thanks for fixing this.

Basically LGTM, I just have one change request.

lib/SILOptimizer/Transforms/DestroyHoisting.cpp

test/SILOptimizer/destroy_hoisting.sil

meg-gupta · 2021-08-20T17:52:25Z

@swift-ci test

meg-gupta · 2021-08-20T21:48:29Z

@swift-ci test

swift-ci · 2021-08-20T23:24:23Z

Build failed
Swift Test Linux Platform
Git Sha - 6534779dbd126422b38ba21cbe5030a0b1d381b6

swift-ci · 2021-08-21T01:10:46Z

Build failed
Swift Test OS X Platform
Git Sha - 6534779dbd126422b38ba21cbe5030a0b1d381b6

eeckstein

thanks, LGTM!

Due to mismatch in the instructions handled in DestroyHoisting::getUsedLocationsOfInst and MemoryLocations::analyzeLocationUsesRecursively, certain users of addresses were not considered and the destroys were hoisted before valid uses causing use-after-frees

meg-gupta · 2021-08-25T20:08:03Z

@swift-ci test

meg-gupta · 2021-08-25T20:08:11Z

@swift-ci benchmark

swift-ci · 2021-08-25T21:41:32Z

Performance (x86_64): -O

Regression	OLD	NEW	DELTA	RATIO
DictionaryOfAnyHashableStrings_insert	3206	5572	+73.8%	0.58x
Set.isDisjoint.Int50	262	337	+28.6%	0.78x (?)
StringFromLongWholeSubstring	4	5	+25.0%	0.80x
DictionaryKeysContainsNative	21	26	+23.8%	0.81x (?)
DictionarySubscriptDefaultMutation	241	269	+11.6%	0.90x (?)
FlattenListLoop	4240	4613	+8.8%	0.92x (?)
Array2D	7216	7808	+8.2%	0.92x
ArrayAppendReserved	1480	1600	+8.1%	0.93x (?)
LessSubstringSubstring	39	42	+7.7%	0.93x (?)
EqualStringSubstring	39	42	+7.7%	0.93x (?)
EqualSubstringSubstringGenericEquatable	39	42	+7.7%	0.93x (?)
EqualSubstringString	39	42	+7.7%	0.93x (?)
LessSubstringSubstringGenericComparable	39	42	+7.7%	0.93x (?)
SortStringsUnicode	2885	3105	+7.6%	0.93x (?)
XorLoop	1890	2033	+7.6%	0.93x (?)

Improvement	OLD	NEW	DELTA	RATIO
Dict.CopyKeyValue.24k	1376	1279	-7.0%	1.08x (?)
RandomTree.insert.Unmanaged.fast	238	222	-6.7%	1.07x (?)

Code size: -O

Regression	OLD	NEW	DELTA	RATIO
FlattenList.o	4058	4106	+1.2%	0.99x

Improvement	OLD	NEW	DELTA	RATIO
ArraySubscript.o	3140	3108	-1.0%	1.01x

Performance (x86_64): -Osize

Regression	OLD	NEW	DELTA	RATIO
StringFromLongWholeSubstring	4	5	+25.0%	0.80x
DictionaryKeysContainsNative	25	29	+16.0%	0.86x (?)
EqualSubstringSubstringGenericEquatable	39	43	+10.3%	0.91x
Array2D	6928	7520	+8.5%	0.92x
XorLoop	1747	1890	+8.2%	0.92x (?)
ArrayPlusEqualFiveElementCollection	7696	8325	+8.2%	0.92x (?)
ArrayAppendReserved	1370	1480	+8.0%	0.93x (?)
ArrayPlusEqualSingleElementCollection	1786	1927	+7.9%	0.93x (?)
RandomShuffleLCG2	416	448	+7.7%	0.93x (?)
LessSubstringSubstring	39	42	+7.7%	0.93x (?)
EqualSubstringSubstring	39	42	+7.7%	0.93x (?)
EqualStringSubstring	39	42	+7.7%	0.93x
EqualSubstringString	39	42	+7.7%	0.93x (?)
LessSubstringSubstringGenericComparable	39	42	+7.7%	0.93x (?)
ArrayAppend	1580	1700	+7.6%	0.93x (?)

Improvement	OLD	NEW	DELTA	RATIO
ObjectiveCBridgeStubFromNSDate	7560	6320	-16.4%	1.20x (?)
ArrayInClass	2000	1865	-6.7%	1.07x (?)

Code size: -Osize

Performance (x86_64): -Onone

Regression	OLD	NEW	DELTA	RATIO
ArrayValueProp	3689	4038	+9.5%	0.91x (?)
ArrayLiteral2	2170	2362	+8.8%	0.92x (?)

Improvement	OLD	NEW	DELTA	RATIO
String.data.Medium	176	150	-14.8%	1.17x (?)
SIMDReduce.Int8x64.Initializer	683490	596652	-12.7%	1.15x (?)
SIMDReduce.Int8x16.Initializer	688263	609026	-11.5%	1.13x (?)
Hanoi	13650	12750	-6.6%	1.07x (?)

Code size: -swiftlibs

Improvement	OLD	NEW	DELTA	RATIO
libswiftStdlibUnittest.dylib	344064	327680	-4.8%	1.05x

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

Remove call to splitCriticalEdges

95215d4

OSSA cannot have critical edges. And DestroyHoisting works only on OSSA.

Make it explicit that MemoryLifetimeVerifier will not handle reborrows

6de9c97

meg-gupta force-pushed the fixdestroyhoisting branch from 3aceea5 to 131e4cd Compare August 19, 2021 22:02

meg-gupta changed the title ~~Fix use-after-free in DestroyHoisting~~ [WIP] Fix use-after-free in DestroyHoisting Aug 20, 2021

eeckstein requested changes Aug 20, 2021

View reviewed changes

meg-gupta force-pushed the fixdestroyhoisting branch 2 times, most recently from 0f8c4ba to 6534779 Compare August 20, 2021 17:50

meg-gupta changed the title ~~[WIP] Fix use-after-free in DestroyHoisting~~ Fix use-after-free in DestroyHoisting Aug 20, 2021

meg-gupta marked this pull request as ready for review August 20, 2021 17:52

eeckstein approved these changes Aug 23, 2021

View reviewed changes

meg-gupta force-pushed the fixdestroyhoisting branch from 6534779 to 1d05105 Compare August 25, 2021 19:58

meg-gupta force-pushed the fixdestroyhoisting branch from 1d05105 to 4c9cb0c Compare August 25, 2021 20:07

meg-gupta merged commit 9ab8763 into swiftlang:main Aug 26, 2021

Fix use-after-free in DestroyHoisting #38965

Fix use-after-free in DestroyHoisting #38965

Uh oh!

Conversation

meg-gupta commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meg-gupta commented Aug 19, 2021

Uh oh!

meg-gupta commented Aug 19, 2021

Uh oh!

meg-gupta commented Aug 19, 2021

Uh oh!

meg-gupta commented Aug 19, 2021

Uh oh!

meg-gupta commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-ci commented Aug 19, 2021

Performance (x86_64): -O

Code size: -O

Performance (x86_64): -Osize

Code size: -Osize

Performance (x86_64): -Onone

Code size: -swiftlibs

Uh oh!

swift-ci commented Aug 19, 2021

Uh oh!

swift-ci commented Aug 20, 2021

Uh oh!

eeckstein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

meg-gupta commented Aug 20, 2021

Uh oh!

meg-gupta commented Aug 20, 2021

Uh oh!

swift-ci commented Aug 20, 2021

Uh oh!

swift-ci commented Aug 21, 2021

Uh oh!

eeckstein left a comment

Choose a reason for hiding this comment

Uh oh!

meg-gupta commented Aug 25, 2021

Uh oh!

meg-gupta commented Aug 25, 2021

Uh oh!

swift-ci commented Aug 25, 2021

Performance (x86_64): -O

Code size: -O

Performance (x86_64): -Osize

Code size: -Osize

Performance (x86_64): -Onone

Code size: -swiftlibs

Uh oh!

Uh oh!

meg-gupta commented Aug 19, 2021 •

edited

Loading

meg-gupta commented Aug 19, 2021 •

edited

Loading