Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1510683002:
Add sk_careful_memcpy to catch undefined behavior in memcpy. (Closed)
DescriptionAdd sk_careful_memcpy to catch undefined behavior in memcpy.
It's undefined behavior to pass null as src or dst to memcpy, even if len is 0.
This currently triggers -fsanitize=attribute-nonnull warnings, but also can
lead to very unexpected code generation with GCC.
sk_careful_memcpy() checks len first before calling memcpy(),
which prevents that weird undefined situation.
This allows me to mark all sanitizers as no-recover, i.e. make-the-bots-red fatal.
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot
BUG=skia:4641
NOTREECHECKS=true
Committed: https://skia.googlesource.com/skia/+/cc881dafcbd00e8a811c47c14b472acdba5dd6c6
Patch Set 1 #Patch Set 2 : memcpy(...,null,0) is not technically legal... #Patch Set 3 : fix ub in MathBench #Patch Set 4 : guard memcpys in SkPathRef::copy #Patch Set 5 : another #Patch Set 6 : disable for sfntly? #Patch Set 7 : two more #
Total comments: 2
Patch Set 8 : sk_careful_memcpy #Patch Set 9 : if(len) #Patch Set 10 : comments #Patch Set 11 : rebase #
Total comments: 2
Patch Set 12 : rebase #Patch Set 13 : reed #
Messages
Total messages: 47 (28 generated)
Description was changed from ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot red, or at least flaky, as some of the sanitizers haven't been causing the bot to go red. BUG=skia:4641 ========== to ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot red, or at least flaky, as some of the sanitizers haven't been causing the bot to go red. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641 ==========
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1510683002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510683002/20001
Description was changed from ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot red, or at least flaky, as some of the sanitizers haven't been causing the bot to go red. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641 ========== to ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot red, or at least flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a null-attribute warning, that memcpy's src array cannot be null. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641 ==========
Description was changed from ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot red, or at least flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a null-attribute warning, that memcpy's src array cannot be null. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641 ========== to ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a null-attribute warning, that memcpy's src array cannot be null. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641 ==========
Description was changed from ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a null-attribute warning, that memcpy's src array cannot be null. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641 ========== to ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a null-attribute warning, that memcpy's src array cannot be null. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot TODO: ../../../third_party/externals/sfntly/sfntly/data/font_data.cc:30:14: runtime error: signed integer overflow: 68 + 2147483647 cannot be represented in type 'int' COMMIT=false BUG=skia:4641 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
Description was changed from ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a null-attribute warning, that memcpy's src array cannot be null. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot TODO: ../../../third_party/externals/sfntly/sfntly/data/font_data.cc:30:14: runtime error: signed integer overflow: 68 + 2147483647 cannot be represented in type 'int' COMMIT=false BUG=skia:4641 ========== to ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a null-attribute warning, that memcpy's src array cannot be null. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot TODO: ../../../third_party/externals/sfntly/sfntly/data/font_data.cc:30:14: runtime error: signed integer overflow: 68 + 2147483647 cannot be represented in type 'int' ../../../bench/MathBench.cpp:130:20: runtime error: signed integer overflow: 1597463007 - -560398336 cannot be represented in type 'int' SUMMARY: AddressSanitizer: undefined-behavior ../../../bench/MathBench.cpp:130:20 COMMIT=false BUG=skia:4641 ==========
Description was changed from ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a null-attribute warning, that memcpy's src array cannot be null. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot TODO: ../../../third_party/externals/sfntly/sfntly/data/font_data.cc:30:14: runtime error: signed integer overflow: 68 + 2147483647 cannot be represented in type 'int' ../../../bench/MathBench.cpp:130:20: runtime error: signed integer overflow: 1597463007 - -560398336 cannot be represented in type 'int' SUMMARY: AddressSanitizer: undefined-behavior ../../../bench/MathBench.cpp:130:20 COMMIT=false BUG=skia:4641 ========== to ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a null-attribute warning, that memcpy's src array cannot be null. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot TODO: ../../../third_party/externals/sfntly/sfntly/data/font_data.cc:30:14: runtime error: signed integer overflow: 68 + 2147483647 cannot be represented in type 'int' ../../../bench/MathBench.cpp:130:20: runtime error: signed integer overflow: 1597463007 - -560398336 cannot be represented in type 'int' SUMMARY: AddressSanitizer: undefined-behavior ../../../bench/MathBench.cpp:130:20 COMMIT=false BUG=skia:4641,skia:4635 ==========
Description was changed from ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a null-attribute warning, that memcpy's src array cannot be null. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot TODO: ../../../third_party/externals/sfntly/sfntly/data/font_data.cc:30:14: runtime error: signed integer overflow: 68 + 2147483647 cannot be represented in type 'int' ../../../bench/MathBench.cpp:130:20: runtime error: signed integer overflow: 1597463007 - -560398336 cannot be represented in type 'int' SUMMARY: AddressSanitizer: undefined-behavior ../../../bench/MathBench.cpp:130:20 COMMIT=false BUG=skia:4641,skia:4635 ========== to ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a null-attribute warning, that memcpy's src array cannot be null. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot TODO: ../../../third_party/externals/sfntly/sfntly/data/font_data.cc:30:14: runtime error: signed integer overflow: 68 + 2147483647 cannot be represented in type 'int' ../../../bench/MathBench.cpp:130:20: runtime error: signed integer overflow: 1597463007 - -560398336 cannot be represented in type 'int' COMMIT=false BUG=skia:4641,skia:4635 ==========
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1510683002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510683002/60001
Description was changed from ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a null-attribute warning, that memcpy's src array cannot be null. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot TODO: ../../../third_party/externals/sfntly/sfntly/data/font_data.cc:30:14: runtime error: signed integer overflow: 68 + 2147483647 cannot be represented in type 'int' ../../../bench/MathBench.cpp:130:20: runtime error: signed integer overflow: 1597463007 - -560398336 cannot be represented in type 'int' COMMIT=false BUG=skia:4641,skia:4635 ========== to ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a few null-attribute warning, that memcpy's src array cannot be null, and integer overflow in the Carmack rsqrt in MathBench.cpp. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot TODO: ../../../third_party/externals/sfntly/sfntly/data/font_data.cc:30:14: runtime error: signed integer overflow: 68 + 2147483647 cannot be represented in type 'int' COMMIT=false BUG=skia:4641,skia:4635 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1510683002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510683002/120001
tomhudson@google.com changed reviewers: + tomhudson@google.com
https://codereview.chromium.org/1510683002/diff/120001/include/core/SkTArray.h File include/core/SkTArray.h (right): https://codereview.chromium.org/1510683002/diff/120001/include/core/SkTArray.... include/core/SkTArray.h:31: inline void copyAndDelete(SkTArray<T, true>* self, char* newMemArray) { Oddly, this doesn't delete anything? https://codereview.chromium.org/1510683002/diff/120001/include/core/SkTDArray.h File include/core/SkTDArray.h (right): https://codereview.chromium.org/1510683002/diff/120001/include/core/SkTDArray... include/core/SkTDArray.h:47: } else if (src.fArray) { Do we not need to invalidate the current contents of this if !src.fArray? (fCount = 0, maybe?)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1510683002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510683002/140001
Description was changed from ========== mark all sanitizers as no-recover (i.e. fatal) This will very likely make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a few null-attribute warning, that memcpy's src array cannot be null, and integer overflow in the Carmack rsqrt in MathBench.cpp. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot TODO: ../../../third_party/externals/sfntly/sfntly/data/font_data.cc:30:14: runtime error: signed integer overflow: 68 + 2147483647 cannot be represented in type 'int' COMMIT=false BUG=skia:4641,skia:4635 ========== to ========== mark all sanitizers as no-recover (i.e. fatal) This may make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a few null-attribute warning, that memcpy's src array cannot be null, and integer overflow in the Carmack rsqrt in MathBench.cpp. Turns off sanitizers for sfntly (we're hitting an integer overflow there). CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641,skia:4635 ==========
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1510683002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510683002/160001
mtklein@chromium.org changed reviewers: + reed@google.com - mtklein@google.com
Have a look now?
Might rename the CL as Add sk_careful_memcpy to catch ... https://codereview.chromium.org/1510683002/diff/200001/include/core/SkTypes.h File include/core/SkTypes.h (right): https://codereview.chromium.org/1510683002/diff/200001/include/core/SkTypes.h... include/core/SkTypes.h:28: // It is undefined behavior to call memcpy() with null dst or src, even if len is 0. Perhaps this (good) comment should be doxygen formatted, and above the function, rather than inside? https://codereview.chromium.org/1510683002/diff/200001/include/core/SkTypes.h... include/core/SkTypes.h:40: if (len) { memcpy(dst,src,len); } I vote for normal LF here if (len) { ... }
Description was changed from ========== mark all sanitizers as no-recover (i.e. fatal) This may make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a few null-attribute warning, that memcpy's src array cannot be null, and integer overflow in the Carmack rsqrt in MathBench.cpp. Turns off sanitizers for sfntly (we're hitting an integer overflow there). CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641,skia:4635 ========== to ========== mark all sanitizers as no-recover (i.e. fatal) This may make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a few null-attribute warning, that memcpy's src array cannot be null. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641,skia:4635 ==========
Description was changed from ========== mark all sanitizers as no-recover (i.e. fatal) This may make the ASAN bot flaky, as some of the sanitizers haven't been causing the bot to go red. Fixes a few null-attribute warning, that memcpy's src array cannot be null. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641,skia:4635 ========== to ========== Add sk_careful_memcpy to catch undefined behavior in memcpy. It's undefined behavior to pass null as src or dst to memcpy, even if len is 0. This currently triggers -fsanitize=attribute-nonnull warnings, but also can lead to very unexpected code generation with GCC. sk_careful_memcpy() checks len first before calling memcpy(), which prevents that weird undefined situation. This allows me to mark all sanitizers as no-recover, i.e. make-the-bots-red fatal. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641 ==========
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
ready for another look
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1510683002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510683002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1510683002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510683002/240001
+CC eric
lgtm
The CQ bit was unchecked by mtklein@google.com
Description was changed from ========== Add sk_careful_memcpy to catch undefined behavior in memcpy. It's undefined behavior to pass null as src or dst to memcpy, even if len is 0. This currently triggers -fsanitize=attribute-nonnull warnings, but also can lead to very unexpected code generation with GCC. sk_careful_memcpy() checks len first before calling memcpy(), which prevents that weird undefined situation. This allows me to mark all sanitizers as no-recover, i.e. make-the-bots-red fatal. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641 ========== to ========== Add sk_careful_memcpy to catch undefined behavior in memcpy. It's undefined behavior to pass null as src or dst to memcpy, even if len is 0. This currently triggers -fsanitize=attribute-nonnull warnings, but also can lead to very unexpected code generation with GCC. sk_careful_memcpy() checks len first before calling memcpy(), which prevents that weird undefined situation. This allows me to mark all sanitizers as no-recover, i.e. make-the-bots-red fatal. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641 NOTREECHECKS=true ==========
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1510683002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510683002/240001
Message was sent while issue was closed.
Description was changed from ========== Add sk_careful_memcpy to catch undefined behavior in memcpy. It's undefined behavior to pass null as src or dst to memcpy, even if len is 0. This currently triggers -fsanitize=attribute-nonnull warnings, but also can lead to very unexpected code generation with GCC. sk_careful_memcpy() checks len first before calling memcpy(), which prevents that weird undefined situation. This allows me to mark all sanitizers as no-recover, i.e. make-the-bots-red fatal. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641 NOTREECHECKS=true ========== to ========== Add sk_careful_memcpy to catch undefined behavior in memcpy. It's undefined behavior to pass null as src or dst to memcpy, even if len is 0. This currently triggers -fsanitize=attribute-nonnull warnings, but also can lead to very unexpected code generation with GCC. sk_careful_memcpy() checks len first before calling memcpy(), which prevents that weird undefined situation. This allows me to mark all sanitizers as no-recover, i.e. make-the-bots-red fatal. CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot BUG=skia:4641 NOTREECHECKS=true Committed: https://skia.googlesource.com/skia/+/cc881dafcbd00e8a811c47c14b472acdba5dd6c6 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://skia.googlesource.com/skia/+/cc881dafcbd00e8a811c47c14b472acdba5dd6c6 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
