fix: clearing user_ip in taos_options_connection does not take effect #33228
Conversation
yihaoDeng
requested review from
dapan1121,
guanshengliang,
hzcheng and
zitsen
as code owners
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 addresses and resolves an error related to clearing user IP addresses by refining the internal mechanism for managing user-specific IP information. It introduces a dedicated field within the connection object to store user IP, ensuring its proper handling and separation from other connection details. Concurrently, the changes include an updated C example program that thoroughly demonstrates the correct usage of connection options for setting and clearing user IP, providing clearer output for verification. Highlights
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue with clearing the user IP by introducing a dedicated field for it and modifying the handling logic. While the core change appears correct, the updated example file (connect_example.c) has several critical issues, including potential buffer overflows, duplicated macro definitions, and a fall-through bug in a switch statement. These should be addressed to ensure the example code is safe and follows good practices.
docs/examples/c/connect_example.c
Outdated
| static int printResult(TAOS_RES *res) { | ||
| int numFields = taos_num_fields(res); | ||
| TAOS_FIELD *fields = taos_fetch_fields(res); | ||
| char header[256] = {0}; |
There was a problem hiding this comment.
The header buffer has a fixed size of 256 bytes. If the combined length of column names in a query result exceeds this limit, sprintf will write past the buffer's boundary, causing a buffer overflow. This is a critical security vulnerability. Consider using dynamic memory allocation to create a buffer of the required size.
docs/examples/c/connect_example.c
Outdated
|
|
||
| TAOS_ROW row = NULL; | ||
| while ((row = taos_fetch_row(res))) { | ||
| char temp[256] = {0}; |
There was a problem hiding this comment.
The temp buffer has a fixed size of 256 bytes. If the string representation of a single row, as formatted by printRow, exceeds this limit, a buffer overflow will occur. This is a critical security vulnerability. A safer approach would be to dynamically allocate memory for the buffer based on the actual required size.
docs/examples/c/connect_example.c
Outdated
| len += sprintf(str + len, "%d", *((int8_t *)row[i])); | ||
| default: |
There was a problem hiding this comment.
The case TSDB_DATA_TYPE_BOOL: is missing a break statement. This causes an unintended fall-through to the default case, which could lead to incorrect behavior.
len += sprintf(str + len, "%d", *((int8_t *)row[i]));
break;
default:
docs/examples/c/connect_example.c
Outdated
| #define varDataLen(v) ((VarDataLenT *)(v))[0] | ||
| #define TSDB_NCHAR_SIZE sizeof(int32_t) | ||
| #define VARSTR_HEADER_SIZE sizeof(VarDataLenT) | ||
|
|
||
| #define GET_FLOAT_VAL(x) (*(float *)(x)) | ||
| #define GET_DOUBLE_VAL(x) (*(double *)(x)) | ||
|
|
There was a problem hiding this comment.
These macro definitions are duplicates of the ones in lines 13-17. Redundant code increases maintenance overhead and can lead to inconsistencies. Please remove these duplicated lines.
docs/examples/c/connect_example.c
Outdated
| { | ||
| { | ||
| TAOS_RES *res = taos_query(taos, "show connections"); | ||
|
|
||
| printResult(res); | ||
| sleep(5); | ||
| if (taos == NULL) { | ||
| fprintf(stderr, "Failed to connect to %s:%hu, ErrCode: 0x%x, ErrMessage: %s.\n", host, port, taos_errno(NULL), | ||
| taos_errstr(NULL)); | ||
| taos_cleanup(); | ||
| return -1; | ||
| } | ||
|
|
||
| taos_free_result(res); | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code for querying connections and printing results is duplicated multiple times in the main function. To improve maintainability and reduce redundancy, this logic should be refactored into a separate helper function.
| } | ||
|
|
||
| if (emptyIpRange(pRange)) return; | ||
| // if (emptyIpRange(pRange)) return; |
There was a problem hiding this comment.
Commenting out this safety check is risky. While it might be intended to handle clearing the user IP, it relies on the implicit behavior of tIpUintToStr when given an empty range. A safer and clearer approach would be to explicitly handle the empty range case to clear the userDualIp field. For example:
if (emptyIpRange(pRange)) {
memset(&connObj->userDualIp, 0, sizeof(connObj->userDualIp));
return;
}
guanshengliang
changed the title
Description
Please briefly describe the code changes in this pull request.
Checklist
Please check the items in the checklist if applicable.