Skip to content

Conversation

@poliudian-iv
Copy link
Contributor

@poliudian-iv poliudian-iv commented Nov 10, 2025

Fix errors found by static analyzer (clangsa), when analyze chromium code. Part 4

icu4c/source/common/putil.cpp - "Value of 'errno' was not checked and may be overwritten by function 'memset'"
icu4c/source/common/ucnv.cpp - "Address of stack memory associated with local variable 'replay' is still referred to by the caller variable 'args' upon returning to the caller. This will be a dangling reference"
icu4c/source/common/udata.cpp - "Null pointer passed to 1st parameter expecting 'nonnull'". We assumed, that 'type' can be null, it follows from checking above
icu4c/source/common/uenum.cpp - "Out of bound access to memory after the end of the field 'data'" if strings without null-terminator
icu4c/source/common/uidna.cpp - "Assigned value is uninitialized" Calling compareCaseInsensitiveASCII without checking errors
icu4c/source/common/uniset.cpp - "Forming reference to null pointer" in UnicodeSet::copyFrom
icu4c/source/common/ushape.cpp - "Out of bound access to memory preceding" in u_shapeArabic and expandCompositCharAtEnd
icu4c/source/i18n/anytrans.cpp - "Access to field 'keyHasher' results in a dereference of a null pointer (loaded from variable 'hash'" at uhash_geti (file icu4c/source/common/uhash.cpp)
icu4c/source/i18n/collationsettings.cpp - "Null pointer passed to " memcpy in CollationSettings::setReorderArrays
icu4c/source/i18n/nfrs.cpp - "Division by zero" in util_lcm
icu4c/source/i18n/nfrule.cpp - "Division by zero" in NFRule::shouldRollBack
icu4c/source/i18n/number_capi.cpp - "Forming reference to null pointer" in UFormattedNumberImpl::setTo
icu4c/source/i18n/number_usageprefs.cpp - "Out of bound access to memory preceding 'zeroMem'" if value.length() < 0
icu4c/source/i18n/scriptset.cpp - "Assigned value is uninitialized" at PropNameData::containsName
icu4c/source/i18n/timezone.cpp - "The right operand of '+' is a garbage value" at TimeZoneFormat::format
icu4c/source/i18n/tzfmt.cpp - as icu4c/source/i18n/timezone.cpp
icu4c/source/i18n/ucol_sit.cpp - "Null pointer passed to 1st parameter expecting 'nonnull'" at uprv_strcat
icu4c/source/i18n/uregex.cpp - "Array access (from variable 'dest') results in a null pointer dereference"

Thank you for your pull request!

TODO: Fill out the checklist below.

Checklist

  • Required: Issue filed: ICU-23267
  • Required: The PR title must be prefixed with a JIRA Issue number. "ICU-23267 Fix static analyzer errors"
  • Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-23267 Fix static analyzer errors"
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable
  • Approver: Feel free to merge on my behalf

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/common/ucnv.cpp is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/test/depstest/dependencies.txt is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/common/ucnv.cpp is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@poliudian-iv poliudian-iv changed the title Fix static analyzer errors ICU-23267 Fix static analyzer errors Nov 13, 2025
@markusicu markusicu self-assigned this Nov 20, 2025
fValue = nullptr;
}
fLength = value.length();
fLength = std::max<int64_t>(0, value.length());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is changed by b88a0d9 because fLength used to be int16_t and cause the int32_t value.length() turn into a negative value -1 and crash if it value.length() is 65535. Please resync the trunk to see is this still needed.

return false;
}
sizeFile = ftell(file);
if (ferror(file) != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (sizeFile == -1)
https://cppreference.com/w/c/io/ftell.html
"Return value
File position indicator on success or -1L if failure occurs."

*/
if (tzInfo->defaultTZBuffer == nullptr) {
rewind(tzInfo->defaultTZFilePtr);
if (ferror(tzInfo->defaultTZFilePtr) != 0) {
Copy link
Contributor

@FrankYFTang FrankYFTang Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed
https://en.cppreference.com/w/c/io/rewind
"The function is equivalent to fseek(stream, 0, SEEK_SET);, except that end-of-file and error indicators are cleared."

How about
change this line to

-                 rewind(tzInfo->defaultTZFilePtr);
+                 if (fseek(tzInfo->defaultTZFilePtr, 0, SEEK_SET) != 0) {

sizeFileRead = fread(tzInfo->defaultTZBuffer, 1, tzInfo->defaultTZFileSize, tzInfo->defaultTZFilePtr);
}
rewind(file);
if (ferror(file) != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, it should be

-            rewind(file);
+            if (fseek(file, 0, SEEK_SET) != 0) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants