Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

connector-local: suppress EPIPE write errors #2316

Merged
merged 2 commits into from Aug 16, 2019

Conversation

@garlick
Copy link
Member

commented Aug 16, 2019

Problem: logs fill up with client_respond() "Broken pipe"
errors when clients disconnect while connector-local is
in the process of sending messages to them.

If EPIPE occurs, call it success and let the read path
handle disconnect errors.

Fixes #2310

@garlick garlick force-pushed the garlick:suppress_epipe branch from c0970f6 to 80e9505 Aug 16, 2019
@grondo

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

I had seen EPIPE errors from another couple read spots in connector-local. This patch quieted them (could be that these were unnecessary, I haven't tried this branch) I was also just addressing errors and not thinking about the "right" fixes here (which is why I didn't add the commit into the previous PR)

diff --git a/src/modules/connector-local/local.c b/src/modules/connector-local/l
index e97c4a2..3a5f855 100644
--- a/src/modules/connector-local/local.c
+++ b/src/modules/connector-local/local.c
@@ -534,8 +534,7 @@ static void request_cb (flux_t *h, flux_msg_handler_t *mh,
                         const flux_msg_t *msg, void *arg)
 {
     client_t *c = arg;
-    if (client_send (c, msg) < 0)
-        flux_log_error (c->ctx->h, "request_cb: client_send");
+    (void) client_send (c, msg);
 }
 
 /* Create a local service, implemented by API client `c`.
@@ -1005,7 +1004,7 @@ static bool internal_request (client_t *c, const flux_msg_
         return false; // no match - forward to broker
 
 done_respond:
-    if (client_respond (c, msg, rc < 0 ? errno : 0) < 0)
+    if (client_respond (c, msg, rc < 0 ? errno : 0) < 0 && errno != EPIPE)
         flux_log_error (c->ctx->h, "internal_req: %s: client_respond", topic);
 done:
     return true;
@@ -1163,7 +1162,9 @@ static void response_cb (flux_t *h, flux_msg_handler_t *mh
     c = zlist_first (ctx->clients);
     while (c) {
         if (!strcmp (uuid, zuuid_str (c->uuid))) {
-            if (client_send_nocopy (c, &cpy) < 0 && allowed_message (c, msg)) {
+            if (client_send_nocopy (c, &cpy) < 0
+                && errno != EPIPE
+                && allowed_message (c, msg)) {
                 int type = FLUX_MSGTYPE_ANY;
                 const char *topic = "unknown";
                 (void)flux_msg_get_type (msg, &type);
garlick added 2 commits Aug 16, 2019
Problem: In response_cb(), if there is a send error,
an error is logged only if allowed_message() returns
true, but it always returns true because responses never
set FLUX_MSGFLAG_PRIVATE.  Furthermore, the restriction on
logging is pointless (presumably it was intended to
restrict delivery).

Remove the check and open #2321 on the possible
security vulnerability it leaves open.
Problem: clients disconnect routinely with response
and event messages in flight, but some of these are
logged to the broker at LOG_ERR level.

Reduce the log noise by suppressing EPIPE errors
in several places.

Fixes #2316.
@garlick garlick force-pushed the garlick:suppress_epipe branch from 80e9505 to 8c66bc3 Aug 16, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

Given that I think the read path handles disconnects and the write path doesn't especially need to also, I just went ahead and suppressed EPIPE LOG_ERR messages at the points identified by @grondo. If we see more log noise we can fix those too.

At one of those points there was a questionable call to allowed_message(). The call was removed and an issue was opened on the possible security issue that was intended to address (though it failed to do so).

@codecov-io

This comment has been minimized.

Copy link

commented Aug 16, 2019

Codecov Report

Merging #2316 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2316      +/-   ##
==========================================
+ Coverage    80.9%   80.91%   +0.01%     
==========================================
  Files         214      214              
  Lines       33815    33815              
==========================================
+ Hits        27358    27363       +5     
+ Misses       6457     6452       -5
Impacted Files Coverage Δ
src/modules/connector-local/local.c 74.42% <100%> (+0.86%) ⬆️
src/common/libflux/message.c 80.62% <0%> (-0.51%) ⬇️
src/broker/module.c 75.17% <0%> (+0.23%) ⬆️
src/cmd/flux-module.c 84.19% <0%> (+0.47%) ⬆️
@grondo grondo merged commit 2982693 into flux-framework:master Aug 16, 2019
4 checks passed
4 checks passed
Summary 1 potential rule
Details
codecov/patch 100% of diff hit (target 80.9%)
Details
codecov/project 80.91% (+0.01%) compared to a70be75
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick garlick referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.