[Reconnoiter-users] [PATCH] noit_rest: fix segfault when running the handoff/journals endpoint

Theo Schlossnagle jesus at omniti.com
Fri Apr 6 17:07:48 EDT 2012


Can you try this?

https://gist.github.com/2322935

Also, if you run valgrind without the --leak-check option, it will be much
less verbose. (We're not looking for leaks here, just bad accesses)

On Thu, Apr 5, 2012 at 3:18 PM, Brandon Philips <brandon at ifup.org> wrote:

> On 14:02 Thu 05 Apr 2012, Theo Schlossnagle wrote:
> > Thanks for the deeper diving in this. Out of curiosity have you run this
> > through valgrind to ensure no leaks.
>
> valgrind on stratcon is really noisy. See below for two outputs.
>
> > I ask b/c you named your variable "can_clean_closure" but you use that
> > variable to prevent calling noit_http_rest_clean_request() which does
> > a lot more than just free the closure.
>
> I am having a hard time understanding the full picture of http handling
> callbacks to evaluate the side effects. So I am trying to fix directly
> the problem of something being freed and then used immediately after.
>
> > Are we doing all that work twice/erroneously or is it really just the
> > closure that is being freed twice?
>
> Valgrind output:
>
> No patch:
> https://gist.github.com/30cb6f76fea2cc1450a2
>
> Patched:
> https://gist.github.com/2647ad6a5e83f7c1f76a
>
> > If it is all that work, I still think there is an issue from the rest
> > callers. From the code, it doesn't look like we're calling the accept
> > closure free twice. A valgrind memcheck output of the double free
> > would be awesome here.
>
> I don't know what you mean. Can you explain how to get a valgrind
> memcheck of the double free? (FWIW, the error isn't a double free, more
> below)
>
> The trouble I was running into is that valgrind slows everything down so
> the fact that we are walking off of the struct into someone elses memory
> no longer segfaults. Thus the need for gdb.
>
> > From the code you changed, it doesn't look like we should be calling
> > the free twice as it was.
>
> It isn't a double free, it is walking around on a free'd pointer.
>
> handoff_stream() calls ac->service_ctx_free() which is set to
> noit_http_rest_closure_free() which frees restc.
>
> Then in noit_rest_request_dispatcher() noit_http_rest_clean_request() is
> called but at this point restc has been freed from the above and is
> usually allocated to something else so everything is invalid that is
> being checked, freed, etc.
>
> > I'm not claiming there isn't a bug, but this seems like it works
> > around an otherwise simple bug with something rather complicated.
>
> This is defiantly a bug. I just don't understand what the intent of the
> switch-e-roo taking place in handoff_stream was so a "simple" fix is
> hard for me to pull off.
>
> > Specifically, in the handoff_ingestor where we free the service_ctx it
> > is an http context (not a rest one), then we flop the rest one into
> > place and set its free function.  So... different ctx and different
> > free function.
>
> Yes, I don't understand how that code works.
>
> This patch is really the best I can do. The problem is clear, my thing
> fixes it by keeping noit_http_rest_clean_request() being called on a
> free'd rest_closure because that is clearly wrong.
>
> Thanks Theo,
>
>        Brandon
>
> > On Thu, Apr 5, 2012 at 1:35 PM, Brandon Philips <brandon at ifup.org>
> wrote:
> >
> > > I improved the patch to do what I think is the right thing instead of
> > > just pointing it out. Please review.
> > >
> > > --
> > >
> > > This fixes a segfault. Here is the problem backtrace:
> > >
> > >    alloc of 0x6d32a0 setting watchpoint @ 0x6d32d0
> > >    Hardware watchpoint 3: * (int *)0x6d32d0
> > >    dealloc of 0x6d32a0 deleting watchpoint
> > >    #0  noit_http_rest_closure_free (v=0x6d32a0) at noit_rest.c:284
> > >    #1  0x00007ffff33614b6 in handoff_stream (restc=0x6d32a0, npats=0,
> > > pats=0x0) at handoff_ingestor.c:253
> > >    #2  0x0000000000428a35 in noit_rest_request_dispatcher
> (ctx=0x6cfce0)
> > > at noit_rest.c:295
> > >    #3  0x0000000000426835 in noit_http_session_drive (e=0x6c0390,
> > > origmask=1, closure=0x6cfce0, now=0x8, done=<optimized out>)
> > >        at noit_http.c:890
> > >    #4  0x0000000000428607 in noit_http_rest_handler (e=0x6c0390,
> mask=1,
> > > closure=0x6c0330, now=0x7fffffffc340) at noit_rest.c:359
> > >    #5  0x000000000041848a in noit_control_dispatch (e=0x6c0390,
> > > mask=<optimized out>, closure=0x6c0330, now=0x7fffffffc340)
> > >        at noit_listener.c:455
> > >    #6  0x000000000043f31e in eventer_epoll_impl_trigger (e=<optimized
> > > out>, mask=1) at eventer_epoll_impl.c:207
> > >    #7  0x000000000043f54b in eventer_epoll_impl_loop () at
> > > eventer_epoll_impl.c:273
> > >    #8  0x0000000000416ab1 in child_main () at stratcond.c:231
> > >    #9  0x0000000000417694 in noit_main (appname=<optimized out>,
> > > config_filename=<optimized out>, debug=-14088,.
> > >        foreground=<optimized out>, _glider=<optimized out>,
> > > drop_to_user=0x7fffffffc900 "", drop_to_group=0x685030 "daemon",.
> > >        passed_child_main=0x4168b0 <child_main>) at noit_main.c:200
> > >    #10 0x0000000000416f78 in main (argc=<optimized out>,
> argv=<optimized
> > > out>) at stratcond.c:237
> > >
> > > And we get into this situation when handling the handoff/journals
> endpoint
> > > and
> > > end up freeing the memory belonging to restc then walking it again in
> > > noit_http_rest_clean_request. Not good.
> > > ---
> > >  src/modules/check_test.c       |    6 ++++--
> > >  src/modules/handoff_ingestor.c |    6 ++++--
> > >  src/modules/httptrap.c         |    8 +++++---
> > >  src/noit_check_rest.c          |   13 ++++++++-----
> > >  src/noit_filters_rest.c        |   10 ++++++----
> > >  src/noit_rest.c                |   12 ++++++++----
> > >  src/noit_rest.h                |    5 +++--
> > >  src/stratcon_datastore.c       |    3 ++-
> > >  src/stratcon_jlog_streamer.c   |    9 ++++++---
> > >  src/stratcon_realtime_http.c   |    3 ++-
> > >  10 files changed, 48 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/src/modules/check_test.c b/src/modules/check_test.c
> > > index 85220ae..bb27e74 100644
> > > --- a/src/modules/check_test.c
> > > +++ b/src/modules/check_test.c
> > > @@ -271,7 +271,8 @@ check_test_schedule_sweeper() {
> > >
> > >  static int
> > >  rest_test_check_err(noit_http_rest_closure_t *restc,
> > > -                    int npats, char **pats) {
> > > +                    int npats, char **pats,
> > > +                    noit_boolean *can_clean_closure) {
> > >   noit_http_response *res =
> noit_http_session_response(restc->http_ctx);
> > >   noit_skiplist_remove(&in_progress, restc,
> > >                        (noit_freefunc_t)rest_test_check_result);
> > > @@ -283,7 +284,8 @@ rest_test_check_err(noit_http_rest_closure_t
> *restc,
> > >  }
> > >  static int
> > >  rest_test_check(noit_http_rest_closure_t *restc,
> > > -                int npats, char **pats) {
> > > +                int npats, char **pats,
> > > +                noit_boolean *can_clean_closure) {
> > >   noit_check_t *tcheck;
> > >   noit_http_session_ctx *ctx = restc->http_ctx;
> > >   int mask, complete = 0;
> > > diff --git a/src/modules/handoff_ingestor.c
> > > b/src/modules/handoff_ingestor.c
> > > index c521f7f..9833525 100644
> > > --- a/src/modules/handoff_ingestor.c
> > > +++ b/src/modules/handoff_ingestor.c
> > > @@ -243,14 +243,16 @@ handoff_http_handler(eventer_t e, int mask, void
> > > *closure,
> > >  }
> > >
> > >  static int
> > > -handoff_stream(noit_http_rest_closure_t *restc, int npats, char
> **pats) {
> > > +handoff_stream(noit_http_rest_closure_t *restc, int npats, char
> **pats,
> > > noit_boolean *can_clean_closure) {
> > >   noit_http_session_ctx *ctx = restc->http_ctx;
> > >   noit_http_connection *conn = noit_http_session_connection(ctx);
> > >   eventer_t e;
> > >   acceptor_closure_t *ac = restc->ac;
> > >
> > > -  if(ac->service_ctx_free)
> > > +  if(ac->service_ctx_free) {
> > >     ac->service_ctx_free(ac->service_ctx);
> > > +    *can_clean_closure = noit_false;
> > > +  }
> > >   ac->service_ctx = ctx;
> > >   ac->service_ctx_free = noit_http_ctx_acceptor_free;
> > >
> > > diff --git a/src/modules/httptrap.c b/src/modules/httptrap.c
> > > index 3cab78e..74e2c1d 100644
> > > --- a/src/modules/httptrap.c
> > > +++ b/src/modules/httptrap.c
> > > @@ -347,7 +347,8 @@ rest_json_payload_free(void *f) {
> > >
> > >  static struct rest_json_payload *
> > >  rest_get_json_upload(noit_http_rest_closure_t *restc,
> > > -                    int *mask, int *complete) {
> > > +                    int *mask, int *complete,
> > > +                    noit_boolean *can_clean_closure) {
> > >   struct rest_json_payload *rxc;
> > >   noit_http_request *req = noit_http_session_request(restc->http_ctx);
> > >   httptrap_closure_t *ccl;
> > > @@ -455,7 +456,8 @@ push_payload_at_check(struct rest_json_payload
> *rxc) {
> > >
> > >  static int
> > >  rest_httptrap_handler(noit_http_rest_closure_t *restc,
> > > -                      int npats, char **pats) {
> > > +                      int npats, char **pats,
> > > +                      noit_boolean *can_clean_closure) {
> > >   int mask, complete = 0, cnt;
> > >   struct rest_json_payload *rxc = NULL;
> > >   const char *error = "internal error", *secret = NULL;
> > > @@ -503,7 +505,7 @@ rest_httptrap_handler(noit_http_rest_closure_t
> *restc,
> > >     restc->call_closure_free = rest_json_payload_free;
> > >   }
> > >
> > > -  rxc = rest_get_json_upload(restc, &mask, &complete);
> > > +  rxc = rest_get_json_upload(restc, &mask, &complete,
> can_clean_closure);
> > >   if(rxc == NULL && !complete) return mask;
> > >
> > >   if(!rxc) goto error;
> > > diff --git a/src/noit_check_rest.c b/src/noit_check_rest.c
> > > index a3fd6af..cfa464e 100644
> > > --- a/src/noit_check_rest.c
> > > +++ b/src/noit_check_rest.c
> > > @@ -120,7 +120,8 @@ noit_check_state_as_xml(noit_check_t *check) {
> > >
> > >  static int
> > >  rest_show_check(noit_http_rest_closure_t *restc,
> > > -                int npats, char **pats) {
> > > +                int npats, char **pats,
> > > +                noit_boolean *can_clean_closure) {
> > >   noit_http_session_ctx *ctx = restc->http_ctx;
> > >   xmlXPathObjectPtr pobj = NULL;
> > >   xmlXPathContextPtr xpath_ctxt = NULL;
> > > @@ -485,7 +486,8 @@ make_conf_path(char *path) {
> > >  }
> > >  static int
> > >  rest_delete_check(noit_http_rest_closure_t *restc,
> > > -                  int npats, char **pats) {
> > > +                  int npats, char **pats,
> > > +                  noit_boolean *can_clean_closure) {
> > >   noit_http_session_ctx *ctx = restc->http_ctx;
> > >   xmlXPathObjectPtr pobj = NULL;
> > >   xmlXPathContextPtr xpath_ctxt = NULL;
> > > @@ -552,7 +554,7 @@ rest_delete_check(noit_http_rest_closure_t *restc,
> > >
> > >  static int
> > >  rest_set_check(noit_http_rest_closure_t *restc,
> > > -               int npats, char **pats) {
> > > +               int npats, char **pats, noit_boolean
> *can_clean_closure) {
> > >   noit_http_session_ctx *ctx = restc->http_ctx;
> > >   xmlXPathObjectPtr pobj = NULL;
> > >   xmlXPathContextPtr xpath_ctxt = NULL;
> > > @@ -663,7 +665,7 @@ rest_set_check(noit_http_rest_closure_t *restc,
> > >   if(pobj) xmlXPathFreeObject(pobj);
> > >   if(doc) xmlFreeDoc(doc);
> > >   restc->fastpath = rest_show_check;
> > > -  return restc->fastpath(restc, restc->nparams, restc->params);
> > > +  return restc->fastpath(restc, restc->nparams, restc->params,
> > > can_clean_closure);
> > >
> > >  error:
> > >   noit_http_response_standard(ctx, error_code, "ERROR", "text/html");
> > > @@ -683,7 +685,8 @@ rest_set_check(noit_http_rest_closure_t *restc,
> > >
> > >  static int
> > >  rest_show_config(noit_http_rest_closure_t *restc,
> > > -                 int npats, char **pats) {
> > > +                 int npats, char **pats,
> > > +                 noit_boolean *can_clean_closure) {
> > >   noit_http_session_ctx *ctx = restc->http_ctx;
> > >   xmlDocPtr doc = NULL;
> > >   xmlNodePtr node, root;
> > > diff --git a/src/noit_filters_rest.c b/src/noit_filters_rest.c
> > > index a91002f..0ceff0c 100644
> > > --- a/src/noit_filters_rest.c
> > > +++ b/src/noit_filters_rest.c
> > > @@ -49,7 +49,7 @@
> > >
> > >  static int
> > >  rest_show_filter(noit_http_rest_closure_t *restc,
> > > -                 int npats, char **pats) {
> > > +                 int npats, char **pats, noit_boolean
> *can_clean_closure)
> > > {
> > >   noit_http_session_ctx *ctx = restc->http_ctx;
> > >   xmlDocPtr doc = NULL;
> > >   xmlNodePtr node, root;
> > > @@ -135,7 +135,8 @@ validate_filter_post(xmlDocPtr doc) {
> > >  }
> > >  static int
> > >  rest_delete_filter(noit_http_rest_closure_t *restc,
> > > -                   int npats, char **pats) {
> > > +                   int npats, char **pats,
> > > +                   noit_boolean *can_clean_closure) {
> > >   noit_http_session_ctx *ctx = restc->http_ctx;
> > >   xmlNodePtr node;
> > >   char xpath[1024];
> > > @@ -175,7 +176,8 @@ rest_delete_filter(noit_http_rest_closure_t *restc,
> > >
> > >  static int
> > >  rest_set_filter(noit_http_rest_closure_t *restc,
> > > -                int npats, char **pats) {
> > > +                int npats, char **pats,
> > > +                noit_boolean *can_clean_closure) {
> > >   noit_http_session_ctx *ctx = restc->http_ctx;
> > >   xmlDocPtr doc = NULL, indoc = NULL;
> > >   xmlNodePtr node, parent, root, newfilter;
> > > @@ -219,7 +221,7 @@ rest_set_filter(noit_http_rest_closure_t *restc,
> > >   restc->call_closure_free = NULL;
> > >   restc->call_closure = NULL;
> > >   restc->fastpath = rest_show_filter;
> > > -  return restc->fastpath(restc, restc->nparams, restc->params);
> > > +  return restc->fastpath(restc, restc->nparams, restc->params,
> > > can_clean_closure);
> > >
> > >  error:
> > >   noit_http_response_standard(ctx, error_code, "ERROR", "text/html");
> > > diff --git a/src/noit_rest.c b/src/noit_rest.c
> > > index b3aa3e6..f89c037 100644
> > > --- a/src/noit_rest.c
> > > +++ b/src/noit_rest.c
> > > @@ -85,7 +85,7 @@ static struct noit_rest_acl *global_rest_acls = NULL;
> > >
> > >  static int
> > >  noit_http_rest_permission_denied(noit_http_rest_closure_t *restc,
> > > -                                 int npats, char **pats) {
> > > +                                 int npats, char **pats, noit_boolean
> > > *can_clean_closure) {
> > >   noit_http_session_ctx *ctx = restc->http_ctx;
> > >   noit_http_response_standard(ctx, 403, "DENIED", "text/xml");
> > >   noit_http_response_end(ctx);
> > > @@ -286,14 +286,17 @@ noit_http_rest_closure_free(void *v) {
> > >
> > >  int
> > >  noit_rest_request_dispatcher(noit_http_session_ctx *ctx) {
> > > +  noit_boolean can_clean_closure = noit_true;
> > >   noit_http_rest_closure_t *restc =
> > > noit_http_session_dispatcher_closure(ctx);
> > >   rest_request_handler handler = restc->fastpath;
> > >   if(!handler) handler = noit_http_get_handler(restc);
> > >    if(handler) {
> > >     noit_http_response *res = noit_http_session_response(ctx);
> > >     int rv;
> > > -    rv = handler(restc, restc->nparams, restc->params);
> > > -    if(noit_http_response_closed(res))
> > > noit_http_rest_clean_request(restc);
> > > +    rv = handler(restc, restc->nparams, restc->params,
> > > &can_clean_closure);
> > > +    if (can_clean_closure == noit_true &&
> noit_http_response_closed(res))
> > > {
> > > +      noit_http_rest_clean_request(restc);
> > > +    }
> > >     return rv;
> > >   }
> > >   noit_http_response_status_set(ctx, 404, "NOT FOUND");
> > > @@ -441,7 +444,8 @@ rest_get_xml_upload(noit_http_rest_closure_t
> *restc,
> > >  }
> > >  int
> > >  noit_rest_simple_file_handler(noit_http_rest_closure_t *restc,
> > > -                              int npats, char **pats) {
> > > +                              int npats, char **pats,
> > > +                              noit_boolean *can_close_closure) {
> > >   int drlen = 0;
> > >   const char *document_root = NULL;
> > >   const char *index_file = NULL;
> > > diff --git a/src/noit_rest.h b/src/noit_rest.h
> > > index 794760c..b38029a 100644
> > > --- a/src/noit_rest.h
> > > +++ b/src/noit_rest.h
> > > @@ -47,7 +47,8 @@
> > >  typedef struct noit_http_rest_closure noit_http_rest_closure_t;
> > >
> > >  typedef int (*rest_request_handler)(noit_http_rest_closure_t *,
> > > -                                    int npats, char **pats);
> > > +                                    int npats, char **pats,
> > > +                                    noit_boolean *);
> > >  typedef noit_boolean
> (*rest_authorize_func_t)(noit_http_rest_closure_t *,
> > >                                               int npats, char **pats);
> > >  struct noit_http_rest_closure {
> > > @@ -87,6 +88,6 @@ API_EXPORT(xmlDocPtr)
> > >
> > >  API_EXPORT(int)
> > >   noit_rest_simple_file_handler(noit_http_rest_closure_t *restc,
> > > -                                int npats, char **pats);
> > > +                                int npats, char **pats, noit_boolean
> *);
> > >
> > >  #endif
> > > diff --git a/src/stratcon_datastore.c b/src/stratcon_datastore.c
> > > index c8afa63..0a102e6 100644
> > > --- a/src/stratcon_datastore.c
> > > +++ b/src/stratcon_datastore.c
> > > @@ -425,7 +425,8 @@ stratcon_datastore_register_onlooker(void
> > > (*f)(stratcon_datastore_op_t,
> > >
> > >  static int
> > >  rest_get_noit_config(noit_http_rest_closure_t *restc,
> > > -                     int npats, char **pats) {
> > > +                     int npats, char **pats,
> > > +                     noit_boolean *can_clean_closure) {
> > >   noit_http_session_ctx *ctx = restc->http_ctx;
> > >   char *xml = NULL;
> > >
> > > diff --git a/src/stratcon_jlog_streamer.c
> b/src/stratcon_jlog_streamer.c
> > > index 3dbfe30..1c0c7b8 100644
> > > --- a/src/stratcon_jlog_streamer.c
> > > +++ b/src/stratcon_jlog_streamer.c
> > > @@ -1205,7 +1205,8 @@ periodic_noit_metrics(eventer_t e, int mask, void
> > > *closure,
> > >
> > >  static int
> > >  rest_show_noits(noit_http_rest_closure_t *restc,
> > > -                int npats, char **pats) {
> > > +                int npats, char **pats,
> > > +                noit_boolean *can_clean_closure) {
> > >   xmlDocPtr doc;
> > >   xmlNodePtr root;
> > >   noit_hash_table seen = NOIT_HASH_EMPTY;
> > > @@ -1489,7 +1490,8 @@ stratcon_remove_noit(const char *target, unsigned
> > > short port) {
> > >  }
> > >  static int
> > >  rest_set_noit(noit_http_rest_closure_t *restc,
> > > -              int npats, char **pats) {
> > > +              int npats, char **pats,
> > > +              noit_boolean *can_clean_closure) {
> > >   const char *cn = NULL;
> > >   noit_http_session_ctx *ctx = restc->http_ctx;
> > >   noit_http_request *req = noit_http_session_request(ctx);
> > > @@ -1511,7 +1513,8 @@ rest_set_noit(noit_http_rest_closure_t *restc,
> > >  }
> > >  static int
> > >  rest_delete_noit(noit_http_rest_closure_t *restc,
> > > -                 int npats, char **pats) {
> > > +                 int npats, char **pats,
> > > +                 noit_boolean *can_clean_closure) {
> > >   noit_http_session_ctx *ctx = restc->http_ctx;
> > >   unsigned short port = 43191;
> > >   if(npats < 1 || npats > 2)
> > > diff --git a/src/stratcon_realtime_http.c
> b/src/stratcon_realtime_http.c
> > > index b05e21a..19764da 100644
> > > --- a/src/stratcon_realtime_http.c
> > > +++ b/src/stratcon_realtime_http.c
> > > @@ -581,7 +581,8 @@ stratcon_realtime_http_handler(eventer_t e, int
> mask,
> > > void *closure,
> > >  }
> > >  static int
> > >  rest_stream_data(noit_http_rest_closure_t *restc,
> > > -                 int npats, char **pats) {
> > > +                 int npats, char **pats,
> > > +                 noit_boolean *can_clean_closure) {
> > >   /* We're here and want to subvert the rest system */
> > >   const char *document_domain = NULL;
> > >   noit_http_session_ctx *ctx = restc->http_ctx;
> > > --
> > > 1.7.0.4
> > >
> > > _______________________________________________
> > > Reconnoiter-users mailing list
> > > Reconnoiter-users at lists.omniti.com
> > > http://lists.omniti.com/mailman/listinfo/reconnoiter-users
> > >
> >
> >
> >
> > --
> >
> > Theo Schlossnagle
> >
> > http://omniti.com/is/theo-schlossnagle
>



-- 

Theo Schlossnagle

http://omniti.com/is/theo-schlossnagle
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.omniti.com/pipermail/reconnoiter-users/attachments/20120406/39aec4d1/attachment-0001.html>


More information about the Reconnoiter-users mailing list