Re: [PATCH] noit_rest: fix segfault when running the handoff/journals endpoint

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  <at>  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
> >  <at>  <at>  -271,7 +271,8  <at>  <at>  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);
> >  <at>  <at>  -283,7 +284,8  <at>  <at>  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
> >  <at>  <at>  -243,14 +243,16  <at>  <at>  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
> >  <at>  <at>  -347,7 +347,8  <at>  <at>  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;
> >  <at>  <at>  -455,7 +456,8  <at>  <at>  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;
> >  <at>  <at>  -503,7 +505,7  <at>  <at>  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
> >  <at>  <at>  -120,7 +120,8  <at>  <at>  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;
> >  <at>  <at>  -485,7 +486,8  <at>  <at>  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;
> >  <at>  <at>  -552,7 +554,7  <at>  <at>  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;
> >  <at>  <at>  -663,7 +665,7  <at>  <at>  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");
> >  <at>  <at>  -683,7 +685,8  <at>  <at>  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
> >  <at>  <at>  -49,7 +49,7  <at>  <at> 
> >
> >  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;
> >  <at>  <at>  -135,7 +135,8  <at>  <at>  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];
> >  <at>  <at>  -175,7 +176,8  <at>  <at>  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;
> >  <at>  <at>  -219,7 +221,7  <at>  <at>  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
> >  <at>  <at>  -85,7 +85,7  <at>  <at>  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);
> >  <at>  <at>  -286,14 +286,17  <at>  <at>  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");
> >  <at>  <at>  -441,7 +444,8  <at>  <at>  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
> >  <at>  <at>  -47,7 +47,8  <at>  <at> 
> >  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 {
> >  <at>  <at>  -87,6 +88,6  <at>  <at>  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
> >  <at>  <at>  -425,7 +425,8  <at>  <at>  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
> >  <at>  <at>  -1205,7 +1205,8  <at>  <at>  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;
> >  <at>  <at>  -1489,7 +1490,8  <at>  <at>  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);
> >  <at>  <at>  -1511,7 +1513,8  <at>  <at>  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
> >  <at>  <at>  -581,7 +581,8  <at>  <at>  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