Additional file handling for randomly generated machine-ids #11185
Conversation
…allback scenario Signed-off-by: dietWall <dietrich7@gmx.de>
Signed-off-by: dietWall <dietrich7@gmx.de>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded a new static helper Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Utils as flb_utils_get_machine_id
participant FS as Filesystem (/tmp/flb-machine-id)
App->>Utils: request machine id
Utils->>FS: check/read `/tmp/flb-machine-id`
alt valid id in file
FS-->>Utils: return sanitized id
else missing/invalid
Utils->>Utils: generate new UUID
Utils->>FS: write_uuid_to_file(new UUID)
alt write succeeds
FS-->>Utils: FLB_TRUE
else write fails
FS-->>Utils: FLB_FALSE (warning logged)
Utils->>Utils: continue using generated UUID
end
end
Utils-->>App: return machine id
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
src/flb_utils.c
Outdated
| if (access(fallback_id_file, F_OK) == 0) | ||
| { | ||
| ret = machine_id_read_and_sanitize(fallback_id_file, &id, &bytes); | ||
| if(ret == 0) | ||
| { | ||
| if (bytes == 0) { | ||
| /* guid is somewhat corrupted */ | ||
| fallback = FLB_TRUE; | ||
| goto fallback; | ||
| } | ||
| } | ||
|
|
||
| *out_id = id; | ||
| *out_size = bytes; | ||
| return 0; |
There was a problem hiding this comment.
When the fallback machine-id file exists but machine_id_read_and_sanitize() returns an error (for example due to permissions or a short read), the code still writes *out_id = id; and *out_size = bytes; even though id/bytes were never assigned. This returns an uninitialized pointer to callers and can lead to crashes or undefined behaviour. The outputs should only be filled after a successful read or the flow should jump to the fallback generation path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I can see this argument, but it should apply for other locations (dbus_var and dbus_etc) as well?
Also testing for bytes==0 does not include any ids with shorter length (what if the file was not written fully?)
I need to check if machine-id files are always in the same format and length.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/flb_utils.c (2)
2046-2069: Useconst char*for parameters and consider matching the reference signature.The parameters
filenameanduuidare not modified within the function, so they should be declared asconst char*. Additionally, the reference implementation inplugins/custom_calyptia/calyptia.cusesflb_sds_tfor the filename parameter, which may provide better consistency across the codebase.Apply this diff to use const parameters:
-int write_uuid_to_file(char* filename, char* uuid) { +int write_uuid_to_file(const char* filename, const char* uuid) {
2114-2129: Minor formatting inconsistency.Line 2117 is missing a space between
ifand the opening parenthesis, which is inconsistent with the rest of the codebase style.Apply this diff:
- if(ret == 0) + if (ret == 0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flb_utils.c(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/flb_utils.c (1)
plugins/custom_calyptia/calyptia.c (1)
write_uuid_to_file(470-493)
src/flb_utils.c
Outdated
| size_t bytes; | ||
| char *uuid; | ||
| int fallback = FLB_FALSE; | ||
| char *fallback_id_file = "machine-id"; //should reside in current working directory |
There was a problem hiding this comment.
Fallback file path is unreliable — use an absolute or well-known directory.
Using "machine-id" without a directory path means the file will be created in the current working directory, which is unpredictable and environment-dependent. This can lead to:
- Permission denied errors in restricted directories
- Files scattered across unexpected locations
- Inability to persist the machine-id reliably across restarts
Consider using a well-defined location such as /var/lib/fluent-bit/machine-id, /tmp/fluent-bit-machine-id, or a configurable path.
Example fix:
- char *fallback_id_file = "machine-id"; //should reside in current working directory
+ char *fallback_id_file = "/var/lib/fluent-bit/machine-id";Note: You may need to ensure the directory exists or handle directory creation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| char *fallback_id_file = "machine-id"; //should reside in current working directory | |
| char *fallback_id_file = "/var/lib/fluent-bit/machine-id"; |
🤖 Prompt for AI Agents
In src/flb_utils.c around line 2078, the hardcoded fallback filename
"machine-id" is unreliable because it creates files in the current working
directory; change it to use a well-defined absolute path or a configurable
location (e.g. /var/lib/fluent-bit/machine-id or /tmp/fluent-bit-machine-id),
ensure the directory exists (create it if necessary with safe permissions), fall
back to a safe temp location if creation fails, and expose the path as a
configurable option so callers can override it.
Signed-off-by: dietWall <dietrich7@gmx.de>
|
implementing suggested changes |
Signed-off-by: dietWall <dietrich7@gmx.de>
…ll/fluent-bit into dietwall/test_machine_id_in_docker
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/flb_utils.c (1)
2079-2128: Fix use of uninitializedid/byteswhen readingfallback_id_filefails.When
machine_id_read_and_sanitize(fallback_id_file, &id, &bytes)returns non‑zero,idandbytesare uninitialized, but the code still assigns them to*out_id/*out_sizeand returns, leading to undefined behaviour and potential crashes. The outputs should only be used onret == 0; otherwise fall back to the UUID path.Also, note that
machine_id_read_and_sanitize()currently dereferencesbuf + bytes - 1even whenbytes == 0, so an empty file can cause out‑of‑bounds access before you ever seebytes == 0here.Suggested restructuring:
- if (access(fallback_id_file, F_OK) == 0){ - ret = machine_id_read_and_sanitize(fallback_id_file, &id, &bytes); - if (ret == 0){ - if (bytes == 0) { - /* guid is somewhat corrupted */ - fallback = FLB_TRUE; - goto fallback; - } - } - - *out_id = id; - *out_size = bytes; - return 0; - } + if (access(fallback_id_file, F_OK) == 0) { + ret = machine_id_read_and_sanitize(fallback_id_file, &id, &bytes); + if (ret == 0) { + if (bytes == 0) { + /* guid is somewhat corrupted */ + fallback = FLB_TRUE; + goto fallback; + } + + *out_id = id; + *out_size = bytes; + return 0; + } + + /* Failed to read/parse fallback file: treat as corrupted and + * fall back to a fresh UUID. */ + fallback = FLB_TRUE; + goto fallback; + }Separately, consider hardening
machine_id_read_and_sanitize()so that a zero‑length file is handled without touchingbuf[-1](e.g., detectbytes == 0right after the read and return an error).
🧹 Nitpick comments (1)
src/flb_utils.c (1)
2232-2241: Guard machine-id persistence to Linux and update stale comment.The review concern is valid. The
fallback_id_fileis defined globally at line 2080 as "/tmp/flb-machine-id" outside all platform guards, and the fallback code at lines 2232-2241 runs on all platforms. Non-Linux systems (FreeBSD, Windows, macOS, etc.) that fall through to the fallback label will attempt to write to/tmp, which is inappropriate for those platforms and will fail with unnecessary warning logs. Additionally, the comment claiming the file "should reside in current working directory" is outdated and contradicts the absolute/tmppath now used.Implement the suggested guard:
- Wrap the write operation (lines 2232-2241) with
#ifdef __linux__/#endif- Update or remove the stale comment at line 2080
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flb_utils.c(4 hunks)
🧰 Additional context used
🪛 ast-grep (0.40.0)
src/flb_utils.c
[warning] 2055-2055: This call makes a world-writable file which allows any user on a machine to write to the file. This may allow attackers to influence the behaviour of this process by writing to the file.
Context: 0666
Note: [CWE-732]: Incorrect Permission Assignment for Critical Resource [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/FIO06-C.+Create+files+with+appropriate+access+permissions
(world-writable-file-c)
Signed-off-by: dietWall <dietrich7@gmx.de>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| } | ||
|
|
||
| *out_id = id; | ||
| *out_size = bytes; | ||
| return 0; |
There was a problem hiding this comment.
If a fallback machine-id file exists but machine_id_read_and_sanitize fails (e.g., unreadable or malformed), ret remains non‑zero yet the code still assigns id/bytes to the outputs and returns success because the return block sits outside the ret == 0 guard. In that scenario id and bytes are uninitialized, so callers can receive a garbage pointer/size or crash instead of regenerating an ID. The return should stay within the successful read path or jump to the fallback path when the read fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/flb_utils.c (1)
2115-2128: Critical: Unguarded assignment of output parameters on read failure.When
machine_id_read_and_sanitizefails (returns non-zero), the code still assigns*out_id = idand*out_size = bytesat lines 2125-2127, even thoughidandbytesmay be uninitialized or contain error values. This can lead to crashes or undefined behavior.Apply this diff to only assign outputs on successful read:
if (access(fallback_id_file, F_OK) == 0){ ret = machine_id_read_and_sanitize(fallback_id_file, &id, &bytes); if (ret == 0){ if (bytes == 0) { /* guid is somewhat corrupted */ fallback = FLB_TRUE; goto fallback; } + *out_id = id; + *out_size = bytes; + return 0; } - - *out_id = id; - *out_size = bytes; - return 0; }
🧹 Nitpick comments (2)
src/flb_utils.c (2)
2046-2070: Use const parameter and wrapped I/O functions for consistency.The
uuidparameter should beconst char *since it's not modified, and the function should useflb_open,flb_write, andflb_closeinstead of the raw POSIX calls for consistency with the similar implementation in the calyptia plugin.Apply this diff:
-static int write_uuid_to_file(const char* filename, char* uuid) +static int write_uuid_to_file(const char* filename, const char* uuid) { int fd; size_t uuid_len; if (filename == NULL || uuid == NULL) { return FLB_FALSE; } /* write uuid to file */ - fd = open(filename, O_CREAT | O_WRONLY | O_TRUNC, 0600); + fd = flb_open(filename, O_CREAT | O_WRONLY | O_TRUNC, 0600); if (fd == -1) { return FLB_FALSE; } uuid_len = strlen(uuid); - if (write(fd, uuid, uuid_len) != uuid_len) { - close(fd); + if (flb_write(fd, uuid, uuid_len) != uuid_len) { + flb_close(fd); return FLB_FALSE; } - close(fd); + flb_close(fd); return FLB_TRUE; }Based on learnings
2079-2079: Remove or correct the misleading comment.The comment states "should reside in current working directory" but the path is absolute (
/tmp/flb-machine-id), not relative to the current working directory.Apply this diff:
- char *fallback_id_file = "/tmp/flb-machine-id"; /*should reside in current working directory*/ + char *fallback_id_file = "/tmp/flb-machine-id";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flb_utils.c(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/flb_utils.c (1)
plugins/custom_calyptia/calyptia.c (1)
write_uuid_to_file(470-493)
🔇 Additional comments (1)
src/flb_utils.c (1)
2234-2241: LGTM: Correct failure handling for file write.The logic correctly checks for write failure with
if (ret == FLB_FALSE)and logs an appropriate warning message. The function continues to return the generated UUID even if persistence fails, which matches the intended behavior described in the PR objectives.
1 participant
This PR adds some additional file handling in case a randomly generated machine-id is required.
This is required for some Unit-Tests in Docker environments, where the tests consistently fails if /etc/machine-id is not mounted. (And mounting it would cause too many issues on other systems)
The behaviour should match the implementation in calyptia plugin, implemented in #9691
More specifically:
first, flb tries to read the machine-id from the default locations (/etc/machine-id, /var/lib/dbus/machine-id)
if none of them exists, on the first run a new machine-id is generated and saved to a file
on subsequent runs, the default locations fail as well. In this case fluent-bit tries to read from the previous file and returns the machine-id that has been generated earlier.
if writing a newly generated uuid to a file fails, fluent-bit would generate a different uuid in subsequent runs, in this case users should see same behaviour as before
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Improvements