config_format: cf_yaml: fix Windows crash when libyaml is loaded as DLL#11402
config_format: cf_yaml: fix Windows crash when libyaml is loaded as DLL#11402bp-cheng wants to merge 4 commits intofluent:masterfrom
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.
The implementation opens the file in binary mode ("rb") for consistent
behavior, reads the entire file content into a buffer, uses
yaml_parser_set_input_string() with the buffer, and properly frees
the buffer after parsing.
Signed-off-by: BP Cheng <bp_cheng@hotmail.com>
Signed-off-by: BP Cheng <bp_cheng@hotmail.com>
📝 WalkthroughWalkthroughReads YAML include files into an in-memory buffer and feeds that buffer to the YAML parser ( Changes
Sequence DiagramsequenceDiagram
participant FS as File System
participant Buffer as Buffer Manager
participant Parser as YAML Parser
FS->>Buffer: open include file (`rb`)
Buffer->>Buffer: stat/determine file size & allocate buffer
FS->>Buffer: read file contents into buffer
Buffer->>Buffer: null-terminate & validate read length
Buffer->>FS: close FILE* (early close for Windows DLL safety)
Buffer->>Parser: yaml_parser_set_input_string(buffer, size)
Parser->>Parser: parse YAML from memory buffer
Parser-->>Buffer: parsing result (success / failure + problem details)
Buffer->>Buffer: free allocated buffer
Parser->>Parser: delete parser if initialized
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: a4a877019c
ℹ️ 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".
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.
The implementation opens the file in binary mode ("rb") for consistent
behavior, reads the entire file content into a buffer, uses
yaml_parser_set_input_string() with the buffer, and properly frees
the buffer after parsing. Error paths now use goto to ensure proper
cleanup of parser state and ctx->level.
Signed-off-by: BP Cheng <bp_cheng@hotmail.com>
Signed-off-by: BP Cheng <bp_cheng@hotmail.com>
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 2963-2971: After calling fread(file_buffer, 1, file_size, fh)
check ferror(fh) (and/or compare bytes_read to file_size as desired) before
closing fh; on error free file_buffer, close fh if open, and return/propagate an
error instead of proceeding. For yaml_parser_initialize(&parser) capture its
return value and only set parser_initialized = 1 when it returns non-zero; if it
fails, free file_buffer, close fh if needed and return an error without calling
yaml_parser_delete or yaml_parser_set_input_string. Reference symbols: fread,
fh, file_buffer, file_size, bytes_read, yaml_parser_initialize,
parser_initialized, yaml_parser_set_input_string, yaml_parser_delete.
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.
The implementation opens the file in binary mode ("rb") for consistent
behavior, reads the entire file content into a buffer, uses
yaml_parser_set_input_string() with the buffer, and properly frees
the buffer after parsing. Error paths now use goto to ensure proper
cleanup of parser state and ctx->level. Added error checks for fread
(via ferror) and yaml_parser_initialize return values.
Signed-off-by: BP Cheng <bp_cheng@hotmail.com>
Signed-off-by: BP Cheng <bp_cheng@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config_format/flb_cf_yaml.c (1)
2840-2841: Guardyaml_event_delete()for early-error paths before parser loop.When early errors occur (file size check, malloc failure, parser initialization failure at lines 2980-2981),
goto doneis executed before anyyaml_parser_parse()call. The event at line 2840 remains uninitialized, but unconditionalyaml_event_delete(&event)at line 3035 violates libyaml's API contract:yaml_event_delete()must only be called after a successfulyaml_parser_parse()call. Initialize event and track successful parse execution to guard the delete.🐛 Proposed fix
Initialize event and only delete after confirmed successful parse:
- yaml_event_t event; + yaml_event_t event = {0}; FILE *fh;And guard the cleanup delete:
- if (code == -1) { - yaml_event_delete(&event); - } + if (code == -1 && event.type != YAML_NO_EVENT) { + yaml_event_delete(&event); + }Or more robustly, only delete if the loop was entered and a parse completed successfully.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4627d30ee
ℹ️ 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".
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.
The implementation opens the file in binary mode ("rb") for consistent
behavior, reads the entire file content into a buffer, uses
yaml_parser_set_input_string() with the buffer, and properly frees
the buffer after parsing.
Signed-off-by: BP Cheng <bp_cheng@hotmail.com>
Signed-off-by: BP Cheng <bp_cheng@hotmail.com>
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.
The implementation opens the file in binary mode ("rb") for consistent
behavior, reads the entire file content into a buffer, uses
yaml_parser_set_input_string() with the buffer, and properly frees
the buffer after parsing.
Signed-off-by: BP Cheng bp_cheng@hotmail.com
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.