config_format: cf_yaml: fix Windows crash when libyaml is loaded as DLL - #11401
config_format: cf_yaml: fix Windows crash when libyaml is loaded as DLL#11401bp-cheng wants to merge 1 commit into
Conversation
On Windows, passing FILE* across DLL boundaries can cause crashes
due to different C runtime libraries having incompatible internal
FILE structures.
When libyaml is built as a dynamic library (DLL), using
yaml_parser_set_input_file() causes the FILE* pointer to cross
the DLL boundary, leading to memory corruption or crashes.
This fix reads the YAML config file into a memory buffer and uses
yaml_parser_set_input_string() instead of yaml_parser_set_input_file(),
avoiding the cross-DLL FILE* issue.
Changes:
- Open file in binary mode ("rb") for consistent behavior
- Read entire file content into a buffer using fseek/ftell/fread
- Use yaml_parser_set_input_string() with the buffer
- Properly free the buffer after parsing
Signed-off-by: bpcheng@hotmail.com
Signed-off-by: BP Cheng <bp_cheng@hotmail.com>
📝 WalkthroughWalkthroughThe PR modifies YAML file reading for include/config files by switching from file-based input to in-memory buffering. Files are read entirely into allocated buffers in binary mode, then parsed via string input to the YAML parser, with proper error handling and cleanup. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b10b9ea31
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /* Read file content */ | ||
| bytes_read = fread(file_buffer, 1, file_size, fh); | ||
| fclose(fh); | ||
| fh = NULL; /* Mark as closed */ | ||
| file_buffer[bytes_read] = '\0'; |
There was a problem hiding this comment.
Treat short reads as errors before parsing
The new buffered read path never checks whether fread consumed the full file_size or whether ferror() was set. If an I/O error or concurrent truncation yields a short read, the parser will consume a truncated buffer and may succeed silently, leaving Fluent Bit running with an incomplete configuration. Please validate bytes_read == file_size (or check ferror) and fail the parse on short reads to preserve the prior behavior of surfacing read errors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/config_format/flb_cf_yaml.c`:
- Around line 2934-2967: After fread(file_buffer, 1, file_size, fh) you must
verify bytes_read == file_size and handle incomplete reads: if bytes_read !=
file_size or ferror(fh) is set, log an error with flb_error including cfg_file
and the bytes_read/file_size values, free file_buffer, close fh if not already
closed, destroy include_dir and include_file, and return -1 to avoid parsing a
truncated config; use the existing symbols file_buffer, bytes_read, file_size,
fread, flb_error, fclose, flb_sds_destroy (and flb_free or equivalent) to
implement the check and cleanup.
| { | ||
| long file_size; | ||
| unsigned char *file_buffer = NULL; | ||
| size_t bytes_read; | ||
|
|
||
| /* Get file size */ | ||
| fseek(fh, 0, SEEK_END); | ||
| file_size = ftell(fh); | ||
| fseek(fh, 0, SEEK_SET); | ||
|
|
||
| if (file_size < 0) { | ||
| flb_error("[config] could not determine file size for %s", cfg_file); | ||
| fclose(fh); | ||
| flb_sds_destroy(include_dir); | ||
| flb_sds_destroy(include_file); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Allocate buffer */ | ||
| file_buffer = flb_malloc(file_size + 1); | ||
| if (!file_buffer) { | ||
| flb_error("[config] could not allocate memory for config file %s", cfg_file); | ||
| fclose(fh); | ||
| flb_sds_destroy(include_dir); | ||
| flb_sds_destroy(include_file); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Read file content */ | ||
| bytes_read = fread(file_buffer, 1, file_size, fh); | ||
| fclose(fh); | ||
| fh = NULL; /* Mark as closed */ | ||
| file_buffer[bytes_read] = '\0'; | ||
|
|
There was a problem hiding this comment.
Add error checking after fread() to detect incomplete reads.
The code reads the file content but doesn't verify that the expected number of bytes were actually read. If fread() encounters an I/O error or EOF before reading file_size bytes, bytes_read will be less than expected, but the code continues silently. This could result in parsing an incomplete configuration file with cryptic YAML errors.
🔍 Proposed fix to add fread validation
/* Read file content */
bytes_read = fread(file_buffer, 1, file_size, fh);
+ if (bytes_read != (size_t)file_size) {
+ flb_error("[config] failed to read complete file %s: expected %ld bytes, got %zu bytes",
+ cfg_file, file_size, bytes_read);
+ if (ferror(fh)) {
+ flb_error("[config] I/O error occurred while reading %s", cfg_file);
+ }
+ flb_free(file_buffer);
+ fclose(fh);
+ flb_sds_destroy(include_dir);
+ flb_sds_destroy(include_file);
+ return -1;
+ }
fclose(fh);
fh = NULL; /* Mark as closed */
file_buffer[bytes_read] = '\0';🤖 Prompt for AI Agents
In `@src/config_format/flb_cf_yaml.c` around lines 2934 - 2967, After
fread(file_buffer, 1, file_size, fh) you must verify bytes_read == file_size and
handle incomplete reads: if bytes_read != file_size or ferror(fh) is set, log an
error with flb_error including cfg_file and the bytes_read/file_size values,
free file_buffer, close fh if not already closed, destroy include_dir and
include_file, and return -1 to avoid parsing a truncated config; use the
existing symbols file_buffer, bytes_read, file_size, fread, flb_error, fclose,
flb_sds_destroy (and flb_free or equivalent) to implement the check and cleanup.
On Windows, passing FILE* across DLL boundaries can cause crashes due to different C runtime libraries having incompatible internal FILE structures.
When libyaml is built as a dynamic library (DLL), using yaml_parser_set_input_file() causes the FILE* pointer to cross the DLL boundary, leading to memory corruption or crashes.
This fix reads the YAML config file into a memory buffer and uses yaml_parser_set_input_string() instead of yaml_parser_set_input_file(), avoiding the cross-DLL FILE* issue.
Changes:
Signed-off-by: bpcheng@hotmail.com
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.