Added default initialization to avoid garbage values#8973
Conversation
| ULONG length = 0; | ||
| UCHAR item; | ||
| USHORT max_segment; | ||
| USHORT max_segment = 0; |
There was a problem hiding this comment.
Wrong initialization. In this case USHRT_MAX is a safe assumption.
There was a problem hiding this comment.
Wrong initialization. In this case
USHRT_MAXis a safe assumption.
In backup.epp, max_segment is initialized with zero in the very same piece of code. If we have no reply for isc_info_blob_max_segment and blob total length is not zero, why should we imply length being USHRT_MAX as you suggest?
There was a problem hiding this comment.
Comment "Rdb sometimes gets the length messed up" suggests that total BLOB length can be evaluated wrongly. In this case using buffer of maximum allowed segment size is better.
I suspect that this piece of code exists only to avoid allocation of 64k memory unconditionally. It had sense in the previous century but not today.
There was a problem hiding this comment.
Its all makes no sense and is a waste of time. Vars gets its values from server response. There can be exactly one reason for any item to have no response - some fatal error. This is API contract. If it become broken, many places in code will fail.
Note, the title say: "...to avoid garbage values" - this code doesn't prevent any kind of garbage values.
Also, why max_segment gets default, while num_segments - not ? Looks like "slightly-pregnant", sorry...
There was a problem hiding this comment.
Because num_segments is not used at all. This routine still can handle only ACLs of max size 64k. Perhaps, @AlexPeshkoff missed it when fixed #2651.
| * | ||
| **************************************/ | ||
| enum trig_t type; | ||
| enum trig_t type = trig_none; |
There was a problem hiding this comment.
For completeness you should also add a check that a meaningful value was assigned below and throw an error if type is still trig_none.
There was a problem hiding this comment.
I'd better use std::optional rather than introduce trig_none for nothing useful.
| AutoCacheRequest request(tdbb, drq_l_rel_info, DYN_REQUESTS); | ||
| QualifiedName masterRelName; | ||
| rel_t masterType, childType; | ||
| rel_t masterType = rel_persistent; |
There was a problem hiding this comment.
This function is basically wrong and will fail in the case if the table has several foreign keys. It must be fixed, not polished.
…ializing to zero each iteration
…Set in get_db_creator
…USHORT with ttype_none as default
| if (desc->isText()) | ||
| { | ||
| if (from_interp == to_interp || to_interp == ttype_none || to_interp == ttype_binary) | ||
| if (from_interp == ttype_none || from_interp == to_interp || to_interp == ttype_none || to_interp == ttype_binary) |
There was a problem hiding this comment.
This may be a wrong change: NONE is the most permissive charset, so conversion from NONE to something else requires validation of data which can be skipped in the other direction.
3qupo
force-pushed
the
added_default_initialization_to_avoid_garbage_values
branch
from
974229b to
d3d0952
Compare
4 participants
This PR adds default initialization for variables that were previously used without being explicitly initialized, potentially leading to undefined behavior due to garbage values.
The changes include:
nullptrfor pointer initializationsThis PR is part of the closed parent PR #8938, which was split into smaller, focused pull requests.