Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(442)

Issue 12079010: Avoid undefined behavior when checking for pointer wraparound (Closed)

Created:
7 years, 11 months ago by nickolai.zeldovich
Modified:
6 years, 4 months ago
CC:
chromium-reviews, erikwright+watch_chromium.org, DO NOT USE (jschuh), jln (very slow on Chromium)
Visibility:
Public.

Description

Avoid undefined behavior when checking for pointer wraparound due to oversized pickled payload. Since pointer wraparound is undefined behavior in C, the current code in Pickle::FindNext(): const char* payload_base = start + header_size; const char* payload_end = payload_base + hdr->payload_size; if (payload_end < payload_base) return NULL; which is equivalent to: if (start + header_size + hdr->payload_size < start + header_size) return NULL; can be simplified by some compilers (e.g., clang) to something like: if (header_size + hdr->payload_size < header_size) return NULL; For example, here's a simplified code snippet and the generated code, which doesn't access the pointer argument at all: nickolai@sahara:/tmp$ cat x.c #include <stdio.h> #include <stdint.h> #include <sys/types.h> void bar(); void foo(const char* start, size_t header_size, uint32_t payload_size) { const char* payload_base = start + header_size; const char* payload_end = payload_base + payload_size; if (payload_end < payload_base) bar(); } nickolai@sahara:/tmp$ clang x.c -S -o - -O3 -m32 ... foo: # @foo # BB#0: movl 8(%esp), %eax movl 12(%esp), %ecx addl %eax, %ecx cmpl %eax, %ecx jge .LBB0_1 # BB#2: jmp bar # TAILCALL .LBB0_1: ret ... nickolai@sahara:/tmp$ Thus, on a 32-bit machine, this can lead to oversized pickle headers bypassing this check and causing undetected pointer wraparound; the caller may then be tricked into accessing memory outside of the pickle payload. The patch replaces the check with one that avoids undefined behavior, and fixes a similar problem in PickleIterator::GetReadPointerAndAdvance pointed out by jar@chromium.org. BUG=none

Patch Set 1 #

Patch Set 2 : #

Total comments: 2

Patch Set 3 : #

Total comments: 2

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -4 lines) Patch
M AUTHORS View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M base/pickle.cc View 1 2 3 4 5 6 7 2 chunks +3 lines, -4 lines 0 comments Download

Messages

Total messages: 30 (0 generated)
nickolai.zeldovich
7 years, 10 months ago (2013-02-06 01:48:17 UTC) #1
jar (doing other things)
https://codereview.chromium.org/12079010/diff/2001/base/pickle.cc File base/pickle.cc (right): https://codereview.chromium.org/12079010/diff/2001/base/pickle.cc#newcode38 base/pickle.cc:38: if (read_ptr_ + sizeof(Type) > read_end_ptr_) Should this to ...
7 years, 10 months ago (2013-02-06 02:47:49 UTC) #2
nickolai.zeldovich
On 2013/02/06 02:47:49, jar wrote: > https://codereview.chromium.org/12079010/diff/2001/base/pickle.cc > File base/pickle.cc (right): > > https://codereview.chromium.org/12079010/diff/2001/base/pickle.cc#newcode38 > ...
7 years, 10 months ago (2013-02-06 03:01:42 UTC) #3
jar (doing other things)
Do you have the patch update? This seems worth finishing and landing. Thanks!!
7 years, 10 months ago (2013-02-26 19:54:31 UTC) #4
nickolai.zeldovich
On 2013/02/26 19:54:31, jar wrote: > Do you have the patch update? This seems worth ...
7 years, 10 months ago (2013-02-26 19:58:40 UTC) #5
nickolai.zeldovich
Hi, Is there anything else you might be waiting for from me on this patch? ...
7 years, 9 months ago (2013-03-04 03:32:06 UTC) #6
jar (doing other things)
I apologize... I missed the patch 2 that you pushed. Comments below. The patch is ...
7 years, 9 months ago (2013-03-04 19:25:51 UTC) #7
nickolai.zeldovich
On 2013/03/04 19:25:51, jar wrote: > (Side note: I was bothered that we currently have ...
7 years, 9 months ago (2013-03-11 00:10:00 UTC) #8
jar (doing other things)
lgtm
7 years, 9 months ago (2013-03-11 04:22:56 UTC) #9
nickolai.zeldovich
On 2013/03/11 04:22:56, jar wrote: > lgtm Any updates on this?
7 years, 5 months ago (2013-07-11 21:44:36 UTC) #10
jar (doing other things)
lgtm My appologies... I approved it... but did not commit. I verified that you signed ...
7 years, 5 months ago (2013-07-11 23:07:56 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nickolai.zeldovich@gmail.com/12079010/15001
7 years, 5 months ago (2013-07-11 23:09:28 UTC) #12
commit-bot: I haz the power
Failed to apply patch for AUTHORS: While running patch -p0 --forward --force --no-backup-if-mismatch; patching file ...
7 years, 5 months ago (2013-07-11 23:09:31 UTC) #13
jar (doing other things)
On 2013/07/11 23:09:31, I haz the power (commit-bot) wrote: > Failed to apply patch for ...
7 years, 5 months ago (2013-07-12 00:26:55 UTC) #14
nickolai.zeldovich
On 2013/07/12 00:26:55, jar wrote: > On 2013/07/11 23:09:31, I haz the power (commit-bot) wrote: ...
7 years, 5 months ago (2013-07-12 14:28:41 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nickolai.zeldovich@gmail.com/12079010/25001
7 years, 5 months ago (2013-07-12 14:46:26 UTC) #16
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=14978
7 years, 5 months ago (2013-07-12 14:56:52 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nickolai.zeldovich@gmail.com/12079010/32001
7 years, 1 month ago (2013-10-31 15:27:38 UTC) #18
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=33671
7 years, 1 month ago (2013-10-31 15:45:49 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nickolai.zeldovich@gmail.com/12079010/32001
7 years, 1 month ago (2013-10-31 15:51:41 UTC) #20
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=33678
7 years, 1 month ago (2013-10-31 16:10:59 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nickolai.zeldovich@gmail.com/12079010/162001
7 years, 1 month ago (2013-10-31 16:13:28 UTC) #22
commit-bot: I haz the power
Retried try job too often on linux_aura for step(s) content_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=92437
7 years, 1 month ago (2013-10-31 17:09:16 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nickolai.zeldovich@gmail.com/12079010/162001
7 years, 1 month ago (2013-11-01 00:12:08 UTC) #24
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) browser_tests, content_unittests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=183445
7 years, 1 month ago (2013-11-01 02:49:17 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nickolai.zeldovich@gmail.com/12079010/602001
7 years, 1 month ago (2013-11-01 02:49:50 UTC) #26
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=217190
7 years, 1 month ago (2013-11-01 04:32:15 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nickolai.zeldovich@gmail.com/12079010/602001
7 years, 1 month ago (2013-11-20 16:39:58 UTC) #28
commit-bot: I haz the power
Failed to apply patch for base/pickle.cc: While running patch -p0 --forward --force --no-backup-if-mismatch; patching file ...
7 years, 1 month ago (2013-11-20 16:40:01 UTC) #29
halyavin
7 years, 1 month ago (2013-11-20 16:42:26 UTC) #30
I found the second bug independently and already committed the fix. This CL
needs to be rebased.

Powered by Google App Engine
This is Rietveld 408576698