Skip to content

enh(transport): add transport safety improvements and test cases #35189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yihaoDeng wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −3
Open

enh(transport): add transport safety improvements and test cases #35189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion source/libs/transport/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ aux_source_directory(src TRANSPORT_SRC)

IF(TD_ENTERPRISE)
LIST(APPEND TRANSPORT_SRC ${TD_ENTERPRISE_DIR}/src/plugins/trans/src/transTLSImpl.c)
#LIST(APPEND TRANSPORT_SRC ${TD_ENTERPRISE_DIR}/src/plugins/trans/src/transSaslImpl.c)
LIST(APPEND TRANSPORT_SRC ${TD_ENTERPRISE_DIR}/src/plugins/trans/src/transSaslImpl.c)
ENDIF()

add_library(transport STATIC ${TRANSPORT_SRC})
Expand Down
5 changes: 4 additions & 1 deletion source/libs/transport/src/transSasl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ void saslBufferClear(SSaslBuffer* buf) {
saslBufferClearImpl(buf);
}

#if !defined(TD_ENTERPRISE)

void saslLibInitImpl() {
return;
}
Expand Down Expand Up @@ -132,6 +134,7 @@ int32_t saslConnHandleAuthImpl(SSaslConn * pConn, const char* input, int32_t len
}

int8_t saslConnShoudDoAuthImpl(SSaslConn * pConn) {
if (pConn == NULL) return 1;
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The return value "1" is used here to signal that authentication should be skipped when "pConn" is NULL. While this correctly prevents a potential crash or connection drop in the caller (e.g., in "transSvr.c"), the use of a magic number in a function named "saslConnShoudDoAuthImpl" is counter-intuitive, as "1" might be expected to mean "True, should do auth". Adding an inline comment would clarify that "1" means "already inited" or "skip".

  if (pConn == NULL) return 1; // Return 1 to indicate auth is finished or not required

return 0;
Comment on lines 136 to 138
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saslConnShoudDoAuthImpl() now treats NULL as "auth initialized" (returns 1), which is needed because several call sites pass conn->saslConn without a NULL check. However this NULL-guard is inside the #if !defined(TD_ENTERPRISE) stub section, so enterprise builds will not get this protection and may crash if enableSasl is off (i.e., saslConn stays NULL). Consider moving the NULL handling into the always-built wrapper saslAuthIsInited() or ensuring the enterprise implementation of saslConnShoudDoAuthImpl() also handles NULL the same way.

Copilot uses AI. Check for mistakes.
}

Expand All @@ -149,4 +152,4 @@ void saslBufferClearImpl(SSaslBuffer* buf) {
return;
}

//#endif
#endif
1 change: 0 additions & 1 deletion source/libs/transport/src/transSvr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,6 @@ int32_t uvWhiteListAdd(SIpWhiteListTab* pWhite, char* user, SIpWhiteListDual* pl
}

pUserList->ver = ver;

pUserList->pList = plist;

code = taosHashPut(pWhiteList, user, strlen(user), &pUserList, sizeof(void*));
Expand Down
2 changes: 2 additions & 0 deletions test/ci/cases.task
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1045,6 +1045,8 @@
,,y,.,./ci/pytest.sh pytest cases/70-Cluster/test_5dnode_3mnode_stop.py -N 5 -M 3
,,y,.,./ci/pytest.sh pytest cases/70-Cluster/test_5dnode_3mnode_stop.py -N 5 -M 3 -I False

# 73-TLS
,,y,.,./ci/pytest.sh pytest cases/73-TLS/test_tls.py
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change adds cases/73-TLS/test_tls.py to the CI task list, but that test currently appears to invoke tlsFileGen.sh using a literal string with {os.path...} braces (missing string interpolation), so the cert/key files likely won't be generated and the TLS-enabled restart may fail. Please either fix the TLS test generation command (and validate it runs in the CI environment) or avoid enabling this case in cases.task until the test is reliable.

Suggested change
,,y,.,./ci/pytest.sh pytest cases/73-TLS/test_tls.py
# Disabled pending fix/validation of cases/73-TLS/test_tls.py TLS file generation in CI.
# ,,y,.,./ci/pytest.sh pytest cases/73-TLS/test_tls.py

Copilot uses AI. Check for mistakes.

# 80-Components

Expand Down
Loading