ticket:13327 Fixing buffer overflow in NumberingSystem constructor.

ticket:13327 Fixing buffer overflow in NumberingSystem constructor.

[sffc, 2017-08-19 02:27:31]

Improving unit test.

[andy.heninger, 2017-09-08 17:52:09]

Mostly LGTM, although the test code could be simplified.

If you patch this into our internal ICU, the internal bug will auto-magically be
fixed. Fuzzing runs continuous re-testing for bugs they have opened.

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++) {
It would be a little simpler to do something like
for (int keywords_size: {ULOC_KEYWORDS_CAPACITY, ULOC_KEYWORDS_CAPACITY+5} ) {
...

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

Otherwise, looks good.

[markus.icu, 2017-09-11 23:05:00]

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?

[sffc, 2017-09-28 00:53:37]

Addressing code review comments.

[sffc, 2017-09-28 00:54:38]

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/11 23:05:00, markus.icu wrote:
> 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.

Since there was a thread about range-based for loops not working, I'll keep it
written the way it is here.

https://codereview.appspot.com/328310043/diff/20001/icu4c/source/test/intltes...
icu4c/source/test/intltest/numfmtst.cpp:8748: localeId[i + 11] = 'x';
On 2017/09/11 23:05:00, markus.icu wrote:
> 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;

I changed it to use CharString like Andy suggested.

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);
On 2017/09/11 23:05:00, markus.icu wrote:
> We have an assertSuccess(), don't we?

Done.

[andy.heninger, 2017-09-28 00:59:05]

LGTM, except for the one comment.

https://codereview.appspot.com/328310043/diff/40001/i18n/numsys.cpp
File i18n/numsys.cpp (right):

https://codereview.appspot.com/328310043/diff/40001/i18n/numsys.cpp#newcode18
i18n/numsys.cpp:18: #include <uassert.h>
quotes, not <angles>

[sffc, 2017-09-28 01:00:13]

Addressing code review comments 2

[sffc, 2017-09-28 01:00:20]

https://codereview.appspot.com/328310043/diff/40001/i18n/numsys.cpp
File i18n/numsys.cpp (right):

https://codereview.appspot.com/328310043/diff/40001/i18n/numsys.cpp#newcode18
i18n/numsys.cpp:18: #include <uassert.h>
On 2017/09/28 00:59:05, andy.heninger wrote:
> quotes, not <angles>

Done.

[sffc, 2017-09-28 01:00:58]

Committed revision 40494.