ICU4C Check type variable on null before dereferencing

[apach301]

I found possible null dereference with Svace static analyzer.

A variable type is checking on nullptr at

icu/icu4c/source/common/udata.cpp

Line 1264 in d805423

if(type!=nullptr && *type!=0) {

.
But later it is used without checking on

icu/icu4c/source/common/udata.cpp

Line 1294 in d805423

if (isICUData && isTimeZoneFile(name, type)) {

.

[CLAassistant]

All committers have signed the CLA.

[richgillam]

@markusicu Double-checked, and I think this is legit.

type is not checked for null at the top of the function; the null check at ll. 1264-1267 only helps with the construction of the pathname that's being constructed there. At least. 1294, we're calling isTimeZoneName(), which is going to call strcmp() on the type parameter to see if it's "res". As I read it, strcmp() isn't null-safe and the behavior is undefined.

This could theoretically also be a problem up at the top of the file, at line 1165, where we call FileTracer::traceOpen(), but when I looked at it, that function is a no-op, which might be why the static analyzer didn't flag that line.

There are other spots further down in the function where type is being passed to a function without null-checking. The few I traced eventually pass it to udata_openChoice(), which passes it to a callback function called isAcceptable(). All the versions of this function I could find ignore this parameter, except for a couple in the unit tests.

I might argue we'd be better off with something like if (type == nullptr) type = ""; (or assertNotNull(type);) at the top of the function, but this does seems like a hole we might want to consider fixing. WDYT?