Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2170)

Issue 8834048: Used size_t for object size instead of signed int. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 1 month ago by Alok Priyadarshi
Modified:
11 years ago
Reviewers:
kbr1, cevans, aedla
CC:
angleproject-review_googlegroups.com
Base URL:
http://angleproject.googlecode.com/svn/trunk/
Visibility:
Public.

Description

Used size_t for object size instead of signed int. BUG=crbug 179653 R=aedla@chromium.org, kbr@chromium.org Committed: https://code.google.com/p/angleproject/source/detail?r=2211

Patch Set 1 #

Total comments: 4

Patch Set 2 : #

Patch Set 3 : removed implicit size_t -> int conversions #

Patch Set 4 : fix build on windows #

Patch Set 5 : rebase with TOT #

Patch Set 6 : clamped object size to INT_MAX #

Unified diffs Side-by-side diffs Delta from patch set Stats (+99 lines, -88 lines) Patch
M src/compiler/Intermediate.cpp View 1 2 3 4 14 chunks +21 lines, -21 lines 0 comments Download
M src/compiler/OutputGLSLBase.cpp View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M src/compiler/OutputHLSL.cpp View 1 2 3 4 5 chunks +18 lines, -13 lines 0 comments Download
M src/compiler/ParseHelper.cpp View 1 2 3 4 6 chunks +8 lines, -16 lines 0 comments Download
M src/compiler/SymbolTable.cpp View 1 2 3 4 5 2 chunks +33 lines, -4 lines 0 comments Download
M src/compiler/Types.h View 1 2 3 4 3 chunks +3 lines, -18 lines 0 comments Download
M src/compiler/intermOut.cpp View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M src/compiler/parseConst.cpp View 1 2 3 4 4 chunks +12 lines, -12 lines 0 comments Download

Messages

Total messages: 18
Alok Priyadarshi
11 years, 1 month ago (2013-04-19 17:57:48 UTC) #1
kbr1
LGTM
11 years, 1 month ago (2013-04-19 18:00:48 UTC) #2
cevans
https://codereview.appspot.com/8834048/diff/1/src/compiler/SymbolTable.cpp File src/compiler/SymbolTable.cpp (right): https://codereview.appspot.com/8834048/diff/1/src/compiler/SymbolTable.cpp#newcode81 src/compiler/SymbolTable.cpp:81: totalSize = size * size; On 32-bit, size_t is ...
11 years, 1 month ago (2013-04-19 18:43:20 UTC) #3
Alok Priyadarshi
https://codereview.appspot.com/8834048/diff/1/src/compiler/SymbolTable.cpp File src/compiler/SymbolTable.cpp (right): https://codereview.appspot.com/8834048/diff/1/src/compiler/SymbolTable.cpp#newcode81 src/compiler/SymbolTable.cpp:81: totalSize = size * size; On 2013/04/19 18:43:21, cevans ...
11 years, 1 month ago (2013-04-19 19:10:28 UTC) #4
aedla
I would like to point out even on 64-bit, getObjectSize() could return anything from 0 ...
11 years ago (2013-04-22 14:44:01 UTC) #5
Alok Priyadarshi
I will check the call-sites to make sure if they really need getObjectSize(). In many ...
11 years ago (2013-04-29 21:56:47 UTC) #6
Alok Priyadarshi
The new patch also removes all implicit conversions. Do you think we still need clamping?
11 years ago (2013-05-01 22:24:16 UTC) #7
aedla
A lot of things can go wrong if getObjectSize returns above INT_MAX. In fact, here ...
11 years ago (2013-05-02 17:59:14 UTC) #8
Alok Priyadarshi
Sorry I do not understand. Did you look at the new patch that removes all ...
11 years ago (2013-05-06 17:31:50 UTC) #9
aedla
Ah sorry, not sure why I didn't notice patch set 3. Maybe I started writing ...
11 years ago (2013-05-06 18:32:34 UTC) #10
Alok Priyadarshi
Any issues with this specific patch?
11 years ago (2013-05-10 16:21:35 UTC) #11
aedla
Overflow checks for getObjectSize and getStructSize should probably be done in this patch as well. ...
11 years ago (2013-05-10 16:58:39 UTC) #12
aedla
One thing that bothers me is memory allocations. There are no checks for allocations returning ...
11 years ago (2013-05-10 19:23:19 UTC) #13
aedla
So yeah, I think we should have the overflow checks, but otherwise this patch LGTM. ...
11 years ago (2013-05-11 14:43:43 UTC) #14
Alok Priyadarshi
On 2013/05/11 14:43:43, aedla wrote: > So yeah, I think we should have the overflow ...
11 years ago (2013-05-14 17:56:45 UTC) #15
aedla
LGTM Clamping might cause problems though. An attacker could create an object with more than ...
11 years ago (2013-05-15 12:45:16 UTC) #16
aedla
Did some more thinking/experimenting. Creating a const object of size INT_MAX is pretty much possible. ...
11 years ago (2013-05-15 13:39:18 UTC) #17
Alok Priyadarshi
11 years ago (2013-05-15 16:46:46 UTC) #18
Message was sent while issue was closed.
Committed patchset #6 manually as r2211 (presubmit successful).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b