Repository navigation
Conversation
| memcpy(*mem, orig_sm_data->shared->schemas[i].mount_point, | ||
| strlen(orig_sm_data->shared->schemas[i].mount_point) + 1); |
There was a problem hiding this comment.
These really do not make sense, I think, so please keep it the way it was.
There was a problem hiding this comment.
Reverted, both are strcpy() again.
| memcpy(*mem, orig_sm_data->shared->schemas[i].content_id, | ||
| strlen(orig_sm_data->shared->schemas[i].content_id) + 1); |
There was a problem hiding this comment.
Similar, does not make sense changing it.
| buf = malloc(strlen(restr->arg.str) + 1); | ||
| LY_CHECK_ERR_GOTO(!buf, LOGMEM(PARSER_CTX(ctx)); ret = LY_EMEM, cleanup); | ||
| strcpy(buf, restr->arg.str); | ||
| memcpy(buf, restr->arg.str, strlen(restr->arg.str) + 1); |
There was a problem hiding this comment.
strdup() should be used instead.
There was a problem hiding this comment.
Done, it uses strdup() now. That needed #define _GNU_SOURCE in parser_yang.c, the same way parser_yin.c has it.
| LYA_FOR(slist->uniques[u], v) { | ||
| if (v) { | ||
| strcpy(ptr, " "); | ||
| memcpy(ptr, " ", 2); |
There was a problem hiding this comment.
Again, makes no sense. Getting rid of strcpy in favor of memcpy just because is not justified, in my opinion.
The schema diff tree, the decimal64 / ipv4-prefix / ipv6-prefix canonical value printers, the date/time helpers and lysc_path_until() still built strings with sprintf()/strcpy() into buffers sized by hand. Each buffer is already large enough, but the writes were unbounded. Convert them to snprintf() bounded by the buffer (sizeof or the allocation size). Output is unchanged. The YANG and YIN pattern modifier parsers duplicated the pattern with malloc() + strcpy(), use strdup() there. The YIN revision-date parser copies LY_REV_SIZE bytes, the length lys_check_date() has just verified.
cc178f6 to
2b69bd2
Compare
|
Thanks for the review. Updated:
What is left is the |
This is a hardening change that bounds the remaining
sprintf()writes insrc/, following the same approach as #2625.What changes
sprintf(...)becomessnprintf(..., <bound>, ...)at every remainingcall site, where
<bound>is the real capacity of the target:sizeof buffor stack buffers,LY_NUMBER_MAXLENfor the numberbuffers allocated with that size.
Files:
schema_diff_tree.c,plugins_types/decimal64.c,plugins_types/ipv4_address_prefix.c,plugins_types/ipv6_address_prefix.c,tree_data_common.c.tree_schema.c:lysc_path_until()writes "/" withsnprintf()boundedby the caller's buffer length instead of
strcpy().parser_yang.candparser_yin.c: the pattern modifier parsers duplicatethe pattern with
strdup()instead ofmalloc()+strcpy().parser_yin.c: the revision-date parser copiesLY_REV_SIZEbytes, thelength
lys_check_date()has just verified, instead ofstrcpy().After review, the
strcpy()calls inplugins_exts/schema_mount.candvalidation.cstay as they are.Not a bug fix
The current size math at these sites is already correct: the formatted
output fits the target in every reachable case, so this does not fix any
observable overflow. It is defense in depth. The value is that the bound
is now stated in the code instead of being an invariant you have to
re-derive.
Testing
Debugbuild with-fsanitize=address,undefined, 0 warnings.ctest72/72 pass under ASan + UBSan, including theformatcheck.