MDEV-30432 replace curl rest connector with libcurl usage#5071
MDEV-30432 replace curl rest connector with libcurl usage#5071grooverdan wants to merge 7 commits into
Conversation
- Remove GetRest occurance - Add libcurl feature to ConnectSE - This patch closes MDEV-26727 (tested with Docker)
…nds" This reverts commit 2a78c3e.
grooverdan
added
the
MariaDB Foundation
![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 replaces the Casablanca (cpprestsdk) dependency and external curl process calls with direct libcurl integration for the CONNECT storage engine's REST support. The changes involve removing restget.cpp, updating build configurations, and implementing new curl_init, curl_run, and curl_deinit methods within the RESTDEF class. Feedback identifies critical thread-safety concerns regarding the placement of global curl initialization, a dangling pointer bug in DefineAM, and potential null pointer dereferences. Further improvements are suggested for file handling robustness, error reporting, URL buffer sizing, and the removal of unused data structures.
| int RESTDEF::curl_init(PGLOBAL g) | ||
| { | ||
| CURLcode curl_res = curl_global_init(CURL_GLOBAL_ALL); | ||
| if (curl_res != CURLE_OK) | ||
| { | ||
| snprintf(g->Message, sizeof(g->Message), | ||
| "unable to initialize curl library, " | ||
| "curl returned this error code: %u " | ||
| "with the following error message: %s", | ||
| curl_res, curl_easy_strerror(curl_res)); | ||
| return 1; | ||
| } | ||
| curl_inited = true; | ||
| return 0; | ||
| } | ||
|
|
||
|
|
||
| /***********************************************************************/ | ||
| /* curl_deinit: Cleanup curl */ | ||
| /***********************************************************************/ | ||
| void RESTDEF::curl_deinit() | ||
| { | ||
| if (curl_inited) | ||
| { | ||
| curl_global_cleanup(); | ||
| curl_inited = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The functions curl_global_init and curl_global_cleanup are not thread-safe and are intended to be called exactly once per process. Calling them within curl_init and curl_deinit (which is called by the RESTDEF destructor) means they will be executed every time a REST table is defined or queried. In a multi-threaded environment like MariaDB, this will lead to race conditions, memory corruption, or crashes if multiple threads access REST tables simultaneously. These calls should be moved to the global plugin initialization and de-initialization routines.
| if (is_error) | ||
| { | ||
| snprintf(g->Message, sizeof(g->Message), "server error"); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
The error message "server error" is too generic. Including the HTTP response code would greatly improve debuggability when a REST request fails with a non-2xx status.
if (is_error)
{
snprintf(g->Message, sizeof(g->Message), "Server returned error code %ld", http_code);
return 1;
}2 participants
Rather than its fragile and gastly use of the curl executable.