Address static analysis warning in UTextProviderLatin1.cpp: Array access results in a null pointer dereference

[Keith Rollin]

Xcode's static analysis reports:

    UTextProviderLatin1.cpp -> darin, myles
    
    /Volumes/Data/dev/webkit/branches/static-analysis/OpenSource/Source/WTF/wtf/text/icu/UTextProviderLatin1.cpp:185:22: warning: Array access (from variable 'dest') results in a null pointer dereference
            dest[length] = 0;
            ~~~~         ^

This seems to be a false positive. Earlier checks involving "destCapacity" will exit the function if "destCapacity" is negative, or if "dest" is NULL and "destCapacity" is positive. Therefore, if "dest" is NULL, we will only enter if "destCapacity" is exactly zero. Further, there is a test just above the cited assignment that will never be true if "destCapacity" is zero. Therefore, the cited assignment will not be executed if "dest" is NULL. Address this by convincing the static analyzer of this with an ASSERT statement.

[Radar WebKit Bug Importer]

<rdar://problem/55672422>

[Keith Rollin]

Created attachment 379466 [details]
Patch

[Geoffrey Garen]

Comment on attachment 379466 [details]
Patch

Is this really a false positive, or does it indicate that the preceding "if (destCapacity > 0 && !dest)" clause is incorrect? (It seems like the early check for "|| (!dest && destCapacity > 0)" should make the check for "if (destCapacity > 0 && !dest)" impossible.

[Keith Rollin]

Huh. Going over email that's been sitting in my Inbox for a while, I reviewed a Coverity Scan that had been sent to me, and saw that it says exactly the same thing:

176         if (destCapacity > 0 && !dest) {
   CID 190890:  Control flow issues  (DEADCODE)
   Execution cannot reach this statement: "trimmedLength = static_cast...".
177             int32_t trimmedLength = static_cast<int32_t>(length);

[Keith Rollin]

I've gone over the function in order to try to understand it. From what you've mentioned and from the Coverity error message, it seems like the if-block protected by the "if (destCapacity > 0 && !dest)" will never be executed. Since this is where the actual characters are being copied, it seems like this function never performed one of its primary functions, which is to extract a sub-string. Indeed, if the if-block *were* executed, the call to StringImpl::copyCharacters would fail since we'd be passing a NULL dest to it.

Examination of the code indicates to me that it's true that uTextLatin1Extract will in fact never be called. That function is part of a family of latin-1 related functions. That family of functions is only used in the context of the ICU function ubrk_setUText, which won't be extracting text. So it looks like uTextLatin1Extract was implemented in the name of completeness only.

Getting back to fixing the function, I think that that if-conditional should instead be "if (destCapacity > 0 && dest)". Or even just "if (dest)". I've made that change, along with adding an "if (dest)" in front of the the "dest[length] = 0" that triggered this whole task. And I took out the ASSERT that I'd added in order to quiet the static analyzer. With those changes, the code compiles and analyzes cleanly.

However, it's not clear if it actually works. It looks like it should work. But it looks like this function was never really used. And also never really tested. Some unit testing should probably be added for UTextProviderLatin1, UTextProviderUTF16, and UTextProvider. But since doing that could generate a whole series of follow-on tasks, I'd like that effort to be part of its own bug and leave this one to addressing the static-analyzer error.

Perhaps this whole function should just be replaced and implemented along the lines of its UTF16 analog? I'll just upload a patch that keeps the existing function largely intact, but could rip it out if people think that's the best approach.

static int32_t uTextUTF16ContextAwareExtract(UText*, int64_t, int64_t, UChar*, int32_t, UErrorCode* errorCode)
{
    // In the present context, this text provider is used only with ICU functions
    // that do not perform an extract operation.
    ASSERT_NOT_REACHED();
    *errorCode = U_UNSUPPORTED_ERROR;
    return 0;
}

[Keith Rollin]

Created attachment 379617 [details]
Patch

[Geoffrey Garen]

Comment on attachment 379617 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 379617 [details]
Patch

Clearing flags on attachment: 379617

Committed r250380: <https://trac.webkit.org/changeset/250380>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.