ICU-22903 Svace: Check values for NULL before access #3195
Conversation
| if (data32!=nullptr) { | ||
| value=enumValue(context, data32[block+j]); | ||
| } else if (idx!=nullptr) { | ||
| value=enumValue(context, idx[block+j]); | ||
| } else { | ||
| /* data32 and idx are not supposed to be NULL at the same time */ | ||
| U_ASSERT(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
No.
- A UTrie2 either stores 32-bit values (via data32), or it stores 16-bit values (via idx). One or the other is not nullptr.
- If this was ever not the case, then we would get a crash -- and we don't.
- If we did want to make this super-duper-safe, but less readable, then we would need to add real error handling here. I don't see it warranted given the invariants.
| if (rbnf != nullptr) { | ||
| res = rbnf->clone(); | ||
| } |
There was a problem hiding this comment.
This does not look right. If the input was a nullptr, then the following code would misleadingly report a memory-allocation-error.
I am fine with guarding against fmt==nullptr although it's a little weird to call a cloning function with a nullptr.
We would need to look at other C API uprefix_clone() functions for consistency:
We could detect fmt==nullptr early, right after the U_FAILURE check, and either just return nullptr, or maybe set a U_ILLEGAL_ARGUMENT_ERROR.
There was a problem hiding this comment.
We would need to look at other C API uprefix_clone() functions for consistency:
We could detect fmt==nullptr early, right after the U_FAILURE check, and either just return nullptr, or maybe set a U_ILLEGAL_ARGUMENT_ERROR.
I don't have a spare time to rework a patch this way, sorry.
Checklist