Fix config access due to race condition #1252
Conversation
Reviewer's Guide by SourceryThis pull request addresses a race condition in the configuration access by using a read-write lock to protect the global configuration. It also refactors the configuration loading and access logic into a separate file, and removes the logging macros from the main source file. Sequence diagram showing thread-safe configuration accesssequenceDiagram
participant T1 as Thread 1
participant T2 as Thread 2
participant CM as ConfigManager
participant C as Config
Note over CM: Using read-write lock for thread safety
T1->>CM: getConfigVal()
activate CM
CM->>CM: pthread_rwlock_rdlock()
CM->>C: Read config value
CM->>CM: pthread_rwlock_unlock()
CM-->>T1: Return value
deactivate CM
T2->>CM: setConfigVal()
activate CM
CM->>CM: pthread_rwlock_wrlock()
CM->>C: Write config value
CM->>CM: pthread_rwlock_unlock()
CM-->>T2: Return status
deactivate CM
Class diagram showing the new configuration and utility structureclassDiagram
class Config {
+char repo_id[MAX_ID_LEN]
+char server_addr[MAX_ADDR_LEN]
+char pub_key[MAX_KEY_LEN]
+char priv_key[MAX_KEY_LEN]
+char server_key[MAX_KEY_LEN]
+char user[MAX_ID_LEN]
+char test_path[MAX_PATH_LEN]
+char log_path[MAX_PATH_LEN]
+char globus_collection_path[MAX_PATH_LEN]
+size_t timeout
}
class ConfigManager {
-Config g_config
-bool config_loaded
-pthread_rwlock_t config_rwlock
+bool initializeGlobalConfig()
+void allowConfigReinitialization()
+bool getConfigVal(char* label, char* dest, size_t max_len)
+bool setConfigVal(char* label, char* src)
+Config createLocalConfigCopy()
}
class Util {
+void uuidToStr(unsigned char* uuid, char* out)
+bool decodeUUID(char* input, char* uuid)
}
class AuthzLog {
+FILE* log_file
+bool write_to_file
+void AUTHZ_LOG_DEBUG()
+void AUTHZ_LOG_INFO()
+void AUTHZ_LOG_ERROR()
+void AUTHZ_LOG_INIT()
+void AUTHZ_LOG_CLOSE()
}
ConfigManager --> Config : manages
note for ConfigManager "Thread-safe configuration management"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@par-hermes format |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @JoshuaSBrown - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
JoshuaSBrown
added
Component: GridFTP Auth Module
| return true; | ||
| } | ||
|
|
||
| strncpy(a_dest, a_src, a_max_len); |
There was a problem hiding this comment.
Should this copy the max length or size_t len? I'm not sure how safe the operation is, but if you provide a larger size than the string is, you could have a memory access issue.
There was a problem hiding this comment.
There was a problem hiding this comment.
That will prevent copying larger data into a smaller string, I am worried about the copy function trying to copy more than the source string has to offer.
e.g.
size_t max = 15;
char* source = "hi";
char destination[max];
strncpy(destination, source, max);where the first two characters of source are valid, but what about the next 13 in this case? It could try to copy that arbitrary memory, I'm not sure
There was a problem hiding this comment.
When you use "" C auto appends the null terminator. So "hi" is actualy 'h' 'i' '\0'.
If you have a char array of just 'h', 'i' yes you will copy in whatever is after that until max is reached.
There was a problem hiding this comment.
I had found incorrect documentation on strncpy, GeeksForGeeks had said the final parameter was the number of characters copied, not the maximum characters copied.
C++ Ref, on the other hand says it's max characters.
Which leads me to another question:
Where are some good online docs for the C standard library?
| } | ||
|
|
||
| // Skip empty lines or comments | ||
| if (*line == '\0' || *line == '#') { |
There was a problem hiding this comment.
Is there any way we could pick up or process non-C strings?
There was a problem hiding this comment.
This is partly why I have the C++ code mixed in as soon as possible. I am not aware of standard string support in the C language. The string.h file only provides utility functions i.e. strlen.
| } | ||
|
|
||
| // Parse the line for key=value | ||
| if (sscanf(line, "%255[^=]=%767s", key, value) != 2) { |
There was a problem hiding this comment.
I'm a little confused by the format string, but it certainly seems to work.
|
|
||
| struct Config createLocalConfigCopy() { | ||
| pthread_rwlock_rdlock(&config_rwlock); | ||
| struct Config temp = g_config; |
There was a problem hiding this comment.
My C is rusty (ha!) and I've been looking at too much JS and Python is assignment like this Copy by default?
There was a problem hiding this comment.
Correct C is copy by default. You have to use & or a pointer for referneces.
| bool getConfigVal(const char *a_label, char *a_dest, size_t a_max_len) { | ||
|
|
||
| // Acquire a read lock (allow concurrent reads) | ||
| pthread_rwlock_rdlock(&config_rwlock); |
| } else if (strcmp(a_label, "globus_collection_path") == 0) { | ||
| strncpy(a_dest, g_config.globus_collection_path, a_max_len); | ||
| } else { | ||
| pthread_rwlock_unlock(&config_rwlock); |
There was a problem hiding this comment.
Looks like we're set up for a single return, and this else can skip the unlock, write to dest and set err=true;
| } else if (strcmp(a_label, "globus_collection_path") == 0) { | ||
| err = setConfigValInternal(a_label, g_config.globus_collection_path, a_src, | ||
| MAX_PATH_LEN); | ||
| } else { |
There was a problem hiding this comment.
The else should likely set err to true
| AUTHZ_LOG_ERROR("Configuration validation failed."); | ||
| // Avoid trying to reload | ||
| config_loaded = true; | ||
| logRelease(); |
There was a problem hiding this comment.
Do we need to release the lock here with pthread_rwlock_unlock?
I'm reading if the lock is still held, the thread can become deadlocked.
There was a problem hiding this comment.
Good catch! Yes, early return will need to be unlocked.
There was a problem hiding this comment.
I think this function can, and should, be refactored to a single return for this reason. Maybe along the lines of
bool err = false;
if (!config_loaded) {
if (config_file) {
while () ...
} else {
err = true;
}
}
...
unlock();
return err;Will also need to set config_loaded accordingly
There was a problem hiding this comment.
Good catches @AronPerez
| if (parseConfigLine(line, line_number)) { | ||
| fclose(config_file); | ||
| // Avoid trying to reload | ||
| config_loaded = true; |
There was a problem hiding this comment.
Question about deadlock here.
| FILE *config_file = openConfigFile("DATAFED_AUTHZ_CFG_FILE", | ||
| "/opt/datafed/authz/datafed-authz.cfg"); | ||
| if (!config_file) { | ||
| return true; |
There was a problem hiding this comment.
Question about deadlock here
|
|
||
| // Initialized only at start | ||
| if (config_loaded) { | ||
| AUTHZ_LOG_INFO("Config file already loaded. Skipping reload.\n"); |
There was a problem hiding this comment.
Question about deadlock here
There was a problem hiding this comment.
Looks to be missing an unlock good catch.
| } else { | ||
| pthread_rwlock_unlock(&config_rwlock); | ||
| return err; | ||
| } | ||
|
|
||
| // Release the write lock | ||
| pthread_rwlock_unlock(&config_rwlock); | ||
| return err; |
There was a problem hiding this comment.
Do we need the else? Seems like we'd release the write lock and return error anyhow.
There was a problem hiding this comment.
If err is initialized as true, you are correct we can get rid of the else good point.
| * @note Assumes read write thread lock is in place. | ||
| */ | ||
| bool validateConfig() { | ||
| char missing[1024] = {0}; |
There was a problem hiding this comment.
Does 1024 refer to some data size?
| * @note Assumes read write thread lock is in place. | ||
| */ | ||
| bool parseConfigLine(const char *line, int line_number) { | ||
| char key[256], value[768]; |
There was a problem hiding this comment.
256 corresponds to the max id length correct? Seems like a magic number that could benefit from an explicit constant name for readability purposes.
Same applies to value value.
| a_max_len); | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
General C question from this.
* This function copies a source string (`a_src`) to a destination buffer
* (`a_dest`) while ensuring the length of the source string does not exceed the
* maximum allowed lengthIs it bad practice to ensure null termination when using strncpy, even if we know we don't exactly need it? There's no scenario where a_src is longer than a_max_len cause
if (len > a_max_len)So no buffer overruns or memory leaks.
I was reading this https://cboard.cprogramming.com/c-programming/117061-null-termination-str-n-cpy-strlen.html#post872229
There was a problem hiding this comment.
This is a really good catch, and demonstrates your attention to detail. You are correct, strlen only returns the number of chars excluding the null pointer. This should raise an error if len + 1 > a_max_len. strncpy will auto append \0 as long as there is room.
There was a problem hiding this comment.
Yeah this is a great catch, I didn't think about the possibility of running out of space for the null terminator during copy.
I'm not sure if it's more clear to write len + 1 > a_max_len or len >= a_max_len or size_t len = strlen(a_src) + 1; but whichever way I feel like it should be noted somewhere nearby the reason for a change.
JoshuaSBrown
force-pushed
the
1241-fix-authz-callout-thrashing-and-logging
branch
from
327bbd2 to
9f8b987
Compare
| m_comm->send(*message); | ||
| response = m_comm->receive(MessageType::GOOGLE_PROTOCOL_BUFFER); | ||
| ++attempt; | ||
| } while (response.time_out && attempt < retries); |
There was a problem hiding this comment.
I like having retries. Is there something that prompted this?
| } | ||
|
|
||
| SDMS::global_logger.addStream(log_file_worker2); | ||
| } |
There was a problem hiding this comment.
Adding a comment so this Debug code doesn't get left in PR, or gets better naming.
PR Description
See issue for what triggered this PR #1241. The PR pulls out the Config structure a C struct with no safety mechanisms to prevent race conditions and memory corruption. The global config file is placed behind thread locks, in addition a switch is placed in front of initializing the Config structure to prevent excessive reads from the config file once the global state has been loaded into memory.
Tasks
Summary by Sourcery
Refactor config loading and access to prevent race conditions and improve thread safety.
Bug Fixes:
Enhancements:
Tests: