Fix/addSafeFunc #34379
Fix/addSafeFunc #34379
Conversation
Summary of ChangesHello @yihaoDeng, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the codebase's resilience against common security vulnerabilities, primarily focusing on memory safety and command execution integrity. By systematically upgrading string handling routines to use safer, bounds-checked functions and introducing robust command validation, the changes enhance the overall stability and security posture of the system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR appears focused on hardening string handling and command execution by replacing unsafe sprintf/strcpy-style usage with bounded APIs (snprintf, tstrncpy, etc.) and adding additional guardrails in a few OS/util helpers.
Changes:
- Replace many
sprintf/tsnprintf/strcpycall sites withsnprintf/tstrncpyto reduce overflow risk. - Add command allowlist/sanitization in
taosOpenCmdto mitigate command injection. - Add some additional bounds checks in test/utility code paths.
Reviewed changes
Copilot reviewed 96 out of 97 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| source/util/src/tstrbuild.c | Switch integer/double appends to snprintf. |
| source/util/src/tsha.c | Replace sprintf with snprintf in test-vector formatting. |
| source/util/src/tcrc32c.c | Add buffer bounds tracking in test main read loop. |
| source/util/src/tconfig.c | Replace tsnprintf with snprintf in config dump helpers. |
| source/os/src/osTime.c | Replace sprintf with snprintf for “NaN” fallback. |
| source/os/src/osSystem.c | Add command allowlist/sanitization; whitespace/style tweaks. |
| source/os/src/osSemaphore.c | Replace tsnprintf with snprintf for /proc path building. |
| source/libs/tss/src/s3.c | Replace tsnprintf/sprintf with snprintf in error/XML building. |
| source/libs/tss/src/fs.c | Replace strcpy with tstrncpy in path joining/copying. |
| source/libs/transport/src/transSvr.c | Use snprintf for whitelist string serialization. |
| source/libs/transport/src/transComm.c | Replace multiple sprintf/tsnprintf with snprintf; remove debug helper. |
| source/libs/transport/src/transCli.c | Replace sprintf with snprintf when setting RPC user. |
| source/libs/transport/src/thttp.c | Replace sprintf with snprintf for endpoint keys. |
| source/libs/tdb/src/db/tdbPager.c | Replace tsnprintf with snprintf for journal filenames. |
| source/libs/sync/test/sync_test_lib/src/syncIO.c | Change binary printing to bounded formatting. |
| source/libs/sync/src/syncUtil.c | Replace tsnprintf with snprintf in log/format helpers. |
| source/libs/sync/src/syncMain.c | Replace tsnprintf with snprintf in config logging. |
| source/libs/scheduler/src/schUtil.c | Replace tsnprintf with snprintf in ep-set dump. |
| source/libs/scalar/src/sclvector.c | Replace tsnprintf with snprintf in var-data conversions. |
| source/libs/scalar/src/sclfunc.c | Replace tsnprintf/sprintf with snprintf in casting/formatting. |
| source/libs/scalar/src/filter.c | Replace tsnprintf with snprintf in filter-to-string/debug output. |
| source/libs/qcom/src/queryUtil.c | Replace tsnprintf with snprintf in data-to-string conversion. |
| source/libs/planner/src/planUtil.c | Replace tsnprintf with snprintf when hashing generated names. |
| source/libs/planner/src/planSpliter.c | Replace tsnprintf with snprintf in generated placeholder names. |
| source/libs/planner/src/planPhysiCreater.c | Minor formatting change around sprintf. |
| source/libs/planner/src/planOptimizer.c | Replace tsnprintf with snprintf in various name builders. |
| source/libs/parser/src/parser.c | Replace sprintf with snprintf while constructing SQL. |
| source/libs/parser/src/parUtil.c | Replace tsnprintf with snprintf in cache key building. |
| source/libs/parser/src/parTranslater.c | Replace
10000
strcpy/tsnprintf with tstrncpy/snprintf in several builders. |
| source/libs/parser/src/parInsertStmt.c | Remove commented-out strcpy lines. |
| source/libs/parser/src/parInsertSql.c | Remove commented-out strcpy lines. |
| source/libs/parser/src/parAstCreater.c | Replace strcpy with tstrncpy for statement fields. |
| source/libs/nodes/src/nodesUtilFuncs.c | Replace tsnprintf with snprintf in dedupe/hash naming. |
| source/libs/nodes/src/nodesToSQLFuncs.c | Replace tsnprintf with snprintf in SQL formatting. |
| source/libs/new-stream/src/streamTriggerTask.c | Replace tsnprintf with snprintf for debug/info buffers. |
| source/libs/new-stream/src/dataSinkFile.c | Replace tsnprintf with snprintf when building temp path. |
| source/libs/monitorfw/src/taos_metric_formatter_custom.c | Replace sprintf with snprintf in log formatting. |
| source/libs/monitorfw/src/taos_metric_formatter.c | Replace sprintf with snprintf for sample formatting. |
| source/libs/monitorfw/src/taos_metric.c | Modify metric name allocation/copy logic. |
| source/libs/monitorfw/src/taos_linked_list.c | Replace sprintf with snprintf in logging. |
| source/libs/monitor/src/monMain.c | Replace tsnprintf with snprintf for timestamp/QID strings. |
| source/libs/monitor/src/monFramework.c | Replace tsnprintf with snprintf for timestamp/QID strings. |
| source/libs/index/src/indexTfile.c | Replace sprintf with snprintf in filename/path generation. |
| source/libs/index/src/indexComm.c | Replace sprintf with snprintf in value-to-string conversion. |
| source/libs/geometry/src/geosWrapper.c | Replace tsnprintf with snprintf for regex pattern formatting. |
| source/libs/function/src/functionMgt.c | Replace tsnprintf with snprintf for hashed function names. |
| source/libs/function/src/builtinsimpl.c | Replace tsnprintf with snprintf in result formatting. |
| source/libs/executor/src/sysscanoperator.c | Replace tsnprintf/sprintf with snprintf in type formatting. |
| source/libs/executor/src/groupcacheoperator.c | Replace tsnprintf with snprintf in debug logging. |
| source/libs/executor/src/forecastoperator.c | Replace tsnprintf with snprintf for generated column names. |
| source/libs/decimal/src/decimal.c | Replace sprintf with snprintf for sign formatting. |
| source/libs/command/src/command.c | Replace tsnprintf with snprintf in SQL output building. |
| source/libs/command/inc/commandInt.h | Replace tsnprintf macros with snprintf variants. |
| source/dnode/vnode/src/vnd/vnodeSvr.c | Replace tsnprintf with snprintf for table name hashing. |
| source/dnode/vnode/src/vnd/vnodeQuery.c | Replace tsnprintf/strcpy with snprintf/tstrncpy. |
| source/dnode/vnode/src/vnd/vnodeOpen.c | Replace tsnprintf with snprintf in path building. |
| source/dnode/vnode/src/tsdb/tsdbReaderWriter.c | Replace sprintf with snprintf for remote path formatting. |
| source/dnode/vnode/src/tsdb/tsdbMigrate.c | Replace sprintf/strcpy with snprintf/tstrncpy in paths. |
| source/dnode/vnode/src/tq/tqOffset.c | Replace tsnprintf with snprintf for path formatting. |
| source/dnode/vnode/src/meta/metaOpen.c | Replace tsnprintf with snprintf for path/log prefixes. |
| source/dnode/vnode/src/bse/bseUtil.c | Comment tweak from sprintf to snprintf. |
| source/dnode/mnode/impl/src/mndVgroup.c | Replace tsnprintf with snprintf for audit formatting. |
| source/dnode/mnode/impl/src/mndUser.c | Replace tsnprintf with snprintf across audit/string builders. |
| source/dnode/mnode/impl/src/mndTrans.c | Replace tsnprintf with snprintf; adjust timestamp formatter signature. |
| source/dnode/mnode/impl/src/mndStreamUtil.c | Replace strcpy with tstrncpy for status message. |
| source/dnode/mnode/impl/src/mndStb.c | Replace tsnprintf with snprintf for audit strings. |
| source/dnode/mnode/impl/src/mndSma.c | Replace tsnprintf with snprintf in SQL/interval formatting. |
| source/dnode/mnode/impl/src/mndScan.c | Replace tsnprintf with snprintf for ep-set detail strings. |
| source/dnode/mnode/impl/src/mndRsma.c | Replace tsnprintf with snprintf in function list formatting. |
| source/dnode/mnode/impl/src/mndRetention.c | Replace tsnprintf with snprintf in audit formatting. |
| source/dnode/mnode/impl/src/mndQnode.c | Replace tsnprintf with snprintf for audit formatting. |
| source/dnode/mnode/impl/src/mndProfile.c | Replace tsnprintf with snprintf in connection/query display. |
| source/dnode/mnode/impl/src/mndMnode.c | Replace tsnprintf with snprintf for audit formatting. |
| source/dnode/mnode/impl/src/mndDnode.c | Replace tsnprintf with snprintf for audit formatting. |
| source/dnode/mnode/impl/src/mndDb.c | Replace tsnprintf with snprintf in retention/keep formatting. |
| source/dnode/mnode/impl/src/mndConfig.c | Replace sprintf/tsnprintf with snprintf for config/audit strings. |
| source/dnode/mnode/impl/src/mndCompact.c | Replace tsnprintf with snprintf in audit formatting. |
| source/dnode/mnode/impl/src/mndBnode.c | Replace tsnprintf with snprintf in endpoint formatting. |
| source/dnode/mnode/impl/src/mndArbGroup.c | Update timestamp helper and related formatting to snprintf. |
| source/dnode/mnode/impl/src/mndAnode.c | Replace tsnprintf with snprintf for URL/name formatting. |
| source/dnode/mgmt/node_util/src/dmEps.c | Replace tsnprintf with snprintf in ep-set to string. |
| source/dnode/mgmt/mgmt_vnode/src/vmHandle.c | Replace tsnprintf with snprintf for mount info copying. |
| source/dnode/mgmt/mgmt_dnode/src/dmHandle.c | Replace sprintf with snprintf when emitting config values. |
| source/dnode/mgmt/exe/dmMain.c | Replace tsnprintf with snprintf for argv logging. |
| source/common/src/ttime.c | Replace sprintf/tsnprintf with snprintf in timestamp formatting. |
| source/common/src/tpriv.c | Replace strcpy with tstrncpy in privilege parsing. |
| source/common/src/tname.c | Replace tsnprintf/sprintf with snprintf in name helpers. |
| source/common/src/tmisce.c | Replace tsnprintf with snprintf in config dump. |
| source/common/src/tglobal.c | Replace sprintf with snprintf during JSON serialization. |
| source/common/src/tdatablock.c | Replace tsnprintf with snprintf in block dumping. |
| source/common/src/tanalytics.c | Replace tsnprintf with snprintf in analytics JSON writing. |
| source/common/src/msg/streamJson.c | Simplify JSON encoding for field name. |
| source/common/src/cos.c | Replace tsnprintf with snprintf in COS/S3 helpers. |
| source/client/src/clientMonitor.c | Replace tsnprintf with snprintf for temp path/audit fields. |
| source/client/src/clientMain.c | Replace tsnprintf with snprintf in row printing. |
| source/client/src/clientEnv.c | Replace tsnprintf with snprintf in string concatenation. |
| include/util/tutil.h | Modify hashing helpers to use bounded formatting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
source/os/src/osTime.c
Outdated
| (void)snprintf(buf, bufSize - 1, "NaN"); | ||
| } |
There was a problem hiding this comment.
In the TD_ASTRA branch, snprintf(buf, bufSize - 1, "NaN") can underflow/behave incorrectly when bufSize is 0 or 1, and it unnecessarily reduces available space. Prefer guarding bufSize > 0 and using snprintf(buf, bufSize, "NaN") so the terminator is always written when possible.
| for (i = 0; i < (int)digest_size; i++) { | ||
| sprintf(output + 2 * i, "%02x", digest[i]); | ||
| snprintf(output + 2 * i, sizeof(output) - 1, "%02x", digest[i]); | ||
| } |
There was a problem hiding this comment.
In the TEST_VECTORS helper, snprintf(output + 2 * i, sizeof(output) - 1, ...) uses the full buffer size for every iteration even though the destination pointer is advanced. That makes the size argument incorrect and can overflow output as i increases. Use the remaining capacity from the current offset (e.g., sizeof(output) - 2*i) or switch back to tsnprintf with the remaining size.
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on enhancing code safety by replacing sprintf and strcpy calls with their safer, size-limited counterparts like snprintf and tstrncpy, and replacing tsnprintf with snprintf. These changes are crucial for preventing potential buffer overflows and improving the overall robustness of the codebase. Additionally, new security measures have been introduced to prevent command injection, and a boundary check was added for a read operation. However, there are a few areas where further attention is needed regarding data initialization and proper usage of snprintf to avoid potential issues.
| self->name = taos_malloc(len); | ||
| memset(self->name, 0, len); | ||
| strcpy(self->name, name); | ||
| self->name[len - 1] = 0; |
There was a problem hiding this comment.
The strcpy(self->name, name); call was removed, but no equivalent function to copy the name string to self->name was added. This means self->name will not be correctly initialized with the provided name, leading to incorrect metric names or undefined behavior. A strncpy or snprintf should be used here to copy the string safely.
tstrncpy(self->name, name, len);| return tsnprintf(pBuf, len, "%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x", ctx.digest[0], | ||
| ctx.digest[1], ctx.digest[2], ctx.digest[3], ctx.digest[4], ctx.digest[5], ctx.digest[6], | ||
| ctx.digest[7], ctx.digest[8], ctx.digest[9], ctx.digest[10], ctx.digest[11], ctx.digest[12], | ||
| ctx.digest[13], ctx.digest[14], ctx.digest[15]); |
There was a problem hiding this comment.
The tsnprintf call here uses len as the size argument. Assuming len represents the total buffer capacity of pBuf, then len is the correct argument for snprintf (or tsnprintf) to ensure null termination and prevent buffer overflows. This is a good change for safety.
include/util/tutil.h
Outdated
| return tsnprintf(pBuf, len - 1, "%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x", | ||
| result[0], result[1], result[2], result[3], result[4], result[5], result[6], result[7], result[8], | ||
| result[9], result[10], result[11], result[12], result[13], result[14], result[15], result[16], | ||
| result[17], result[18], result[19]); | ||
| } |
There was a problem hiding this comment.
The tsnprintf call here uses len - 1 as the size argument. If len is the total buffer capacity of pBuf, then len should be passed as the size argument to snprintf (or tsnprintf) to allow it to write up to len-1 characters and null-terminate. Using len - 1 might unnecessarily reduce the available buffer space by one byte. Please confirm the intended buffer size for pBuf and adjust accordingly.
return tsnprintf(pBuf, len, "%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x",
result[0], result[1], result[2], result[3], result[4], result[5], result[6], result[7], result[8],
result[9], result[10], result[11], result[12], result[13], result[14], result[15], result[16],
result[17], result[18], result[19]);
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.