metrics: add support for metrics over UNIX socket#494
metrics: add support for metrics over UNIX socket#494
Conversation
mmuzila
force-pushed
the
add_unix_socket_metrics
branch
from
3de57d0 to
6b21c72
Compare
📝 WalkthroughWalkthroughMetrics address parsing now accepts Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@main/metrics.c`:
- Around line 314-319: The end cleanup double-closes fd when evconnlistener_new
was called with LEV_OPT_CLOSE_ON_FREE because the listener owns and will close
fd; modify the end block so you only close fd when the listener was not created
(i.e., if listener == NULL and fd >= 0), keeping evconnlistener_free(listener)
as-is when listener != NULL; reference the symbols listener, fd,
evconnlistener_new, evconnlistener_free and the LEV_OPT_CLOSE_ON_FREE option to
locate the creation/cleanup logic and put the close(fd) inside the else branch
(or guard it with if (listener == NULL)) so ownership isn’t released twice.
| end: | ||
| if (listener != NULL) | ||
| evconnlistener_free(listener); | ||
| if (fd >= 0) | ||
| close(fd); | ||
| return errno; |
There was a problem hiding this comment.
Double-close of file descriptor when listener is successfully created.
When evconnlistener_new succeeds with LEV_OPT_CLOSE_ON_FREE, the listener takes ownership of fd and will close it in evconnlistener_free(listener). The subsequent close(fd) on line 318 then double-closes the descriptor, which can corrupt other threads' file descriptors if the number gets reused.
🐛 Proposed fix
end:
- if (listener != NULL)
+ if (listener != NULL) {
evconnlistener_free(listener);
- if (fd >= 0)
- close(fd);
+ } else if (fd >= 0) {
+ close(fd);
+ }
return errno;📝 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.
| end: | |
| if (listener != NULL) | |
| evconnlistener_free(listener); | |
| if (fd >= 0) | |
| close(fd); | |
| return errno; | |
| end: | |
| if (listener != NULL) { | |
| evconnlistener_free(listener); | |
| } else if (fd >= 0) { | |
| close(fd); | |
| } | |
| return errno; |
🤖 Prompt for AI Agents
In `@main/metrics.c` around lines 314 - 319, The end cleanup double-closes fd when
evconnlistener_new was called with LEV_OPT_CLOSE_ON_FREE because the listener
owns and will close fd; modify the end block so you only close fd when the
listener was not created (i.e., if listener == NULL and fd >= 0), keeping
evconnlistener_free(listener) as-is when listener != NULL; reference the symbols
listener, fd, evconnlistener_new, evconnlistener_free and the
LEV_OPT_CLOSE_ON_FREE option to locate the creation/cleanup logic and put the
close(fd) inside the else branch (or guard it with if (listener == NULL)) so
ownership isn’t released twice.
260b200 to
3af4e3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@main/metrics.c`:
- Around line 273-276: The code currently uses strncpy to copy
gr_config.metrics_addr into saddr.sun_path which can silently truncate long
paths; before copying, check the length of gr_config.metrics_addr against
sizeof(saddr.sun_path) (referencing saddr, saddr.sun_path, and
gr_config.metrics_addr) and if it is too long return an error/log and abort
binding; if it fits, copy safely and ensure null-termination (or use snprintf
with the buffer size) so the process never binds to a truncated/unintended
socket path.
| memset(&saddr, 0, sizeof(saddr)); | ||
| saddr.sun_family = AF_UNIX; | ||
| strncpy(saddr.sun_path, gr_config.metrics_addr, sizeof(saddr.sun_path) - 1); | ||
|
|
There was a problem hiding this comment.
Silent truncation of UNIX socket path can cause binding to unintended socket.
If gr_config.metrics_addr exceeds 107 characters (Linux sun_path limit is typically 108), strncpy silently truncates the path. The application would then bind to a different socket path than the user specified, which could cause operational failures or security issues.
🐛 Proposed fix: validate path length before use
+ if (strlen(gr_config.metrics_addr) >= sizeof(saddr.sun_path)) {
+ LOG(ERR, "metrics socket path too long: %s", gr_config.metrics_addr);
+ errno = ENAMETOOLONG;
+ goto end;
+ }
+
memset(&saddr, 0, sizeof(saddr));
saddr.sun_family = AF_UNIX;
strncpy(saddr.sun_path, gr_config.metrics_addr, sizeof(saddr.sun_path) - 1);📝 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.
| memset(&saddr, 0, sizeof(saddr)); | |
| saddr.sun_family = AF_UNIX; | |
| strncpy(saddr.sun_path, gr_config.metrics_addr, sizeof(saddr.sun_path) - 1); | |
| if (strlen(gr_config.metrics_addr) >= sizeof(saddr.sun_path)) { | |
| LOG(ERR, "metrics socket path too long: %s", gr_config.metrics_addr); | |
| errno = ENAMETOOLONG; | |
| goto end; | |
| } | |
| memset(&saddr, 0, sizeof(saddr)); | |
| saddr.sun_family = AF_UNIX; | |
| strncpy(saddr.sun_path, gr_config.metrics_addr, sizeof(saddr.sun_path) - 1); |
🤖 Prompt for AI Agents
In `@main/metrics.c` around lines 273 - 276, The code currently uses strncpy to
copy gr_config.metrics_addr into saddr.sun_path which can silently truncate long
paths; before copying, check the length of gr_config.metrics_addr against
sizeof(saddr.sun_path) (referencing saddr, saddr.sun_path, and
gr_config.metrics_addr) and if it is too long return an error/log and abort
binding; if it fits, copy safely and ensure null-termination (or use snprintf
with the buffer size) so the process never binds to a truncated/unintended
socket path.
Previously metrics were available over tcp/http only, this change introduces metrics over UNIX socket. The format of `-M` parameter was `ADDRESS:PORT`, this commit changes its format to `[tcp:]ADDRESS:PORT | unix:PATH`. Signed-off-by: Matej Mužila <mmuzila@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
grout-bot
force-pushed
the
add_unix_socket_metrics
branch
from
3af4e3e to
d527e26
Compare
rjarry
left a comment
rjarry
left a comment
There was a problem hiding this comment.
Could you rework to allow listening to HTTP on the unix socket as well? Not just plain TCP.
3 participants
Previously metrics were available over tcp/http only, this change introduces metrics over UNIX socket.
The format of
-Mparameter wasADDRESS:PORT, this commit changes its format to[tcp:]ADDRESS:PORT | unix:PATH.Summary by CodeRabbit
New Features
Bug Fixes
Documentation