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

Issue 328310043: ticket:13327 Fixing buffer overflow in NumberingSystem constructor.

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month ago by sffc
Modified:
1 week, 6 days ago
CC:
nrunge
Base URL:
svn+icussh://source.icu-project.org/repos/icu/trunk/
Visibility:
Public.

Description

ticket:13327 Fixing buffer overflow in NumberingSystem constructor.

Patch Set 1 #

Patch Set 2 : Improving unit test. #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -0 lines) Patch
M icu4c/source/i18n/numsys.cpp View 2 chunks +7 lines, -0 lines 0 comments Download
M icu4c/source/test/intltest/numfmtst.h View 1 chunk +1 line, -0 lines 0 comments Download
M icu4c/source/test/intltest/numfmtst.cpp View 1 2 chunks +20 lines, -0 lines 4 comments Download

Messages

Total messages: 4
sffc
1 month ago (2017-08-19 02:21:42 UTC) #1
sffc
Improving unit test.
1 month ago (2017-08-19 02:27:31 UTC) #2
andy.heninger
Mostly LGTM, although the test code could be simplified. If you patch this into our ...
2 weeks, 2 days ago (2017-09-08 17:52:09 UTC) #3
markus.icu
1 week, 6 days ago (2017-09-11 23:05:00 UTC) #4
https://codereview.appspot.com/328310043/diff/20001/icu4c/source/test/intltes...
File icu4c/source/test/intltest/numfmtst.cpp (right):

https://codereview.appspot.com/328310043/diff/20001/icu4c/source/test/intltes...
icu4c/source/test/intltest/numfmtst.cpp:8741: for (int runId = 0; runId < 2;
runId++) {
On 2017/09/08 17:52:09, andy.heninger wrote:
> It would be a little simpler to do something like
> for (int keywords_size: {ULOC_KEYWORDS_CAPACITY, ULOC_KEYWORDS_CAPACITY+5} ) {
> ...

I agree, but I don't care strongly.

> and to use an ICU CharString or a std::string to build up the localeId.

Sure, but for a fixed length writing to a simple array is fine too.

https://codereview.appspot.com/328310043/diff/20001/icu4c/source/test/intltes...
icu4c/source/test/intltest/numfmtst.cpp:8748: localeId[i + 11] = 'x';
This +11 looks a little fragile.
I would
    char localeID[capacity] = "en@numbers=";
    char *kw = strchr(localeID, 0);
    int i;
    for (i = 0; i < ...; ++i) {
        kw[i] = 'x';
    }
    kw[i] = 0;

https://codereview.appspot.com/328310043/diff/20001/icu4c/source/test/intltes...
icu4c/source/test/intltest/numfmtst.cpp:8754: assertEquals("Should create with
no error", U_ZERO_ERROR, status);
We have an assertSuccess(), don't we?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 80a51fa-tainted