coredump problem of short connection accept.

[helinbo2015]

1. coredump problem of short connection:

use libevhtp as http server, if request from client is a short connection(include "Connection":"close" header), i got a coredump at call htparser_get_error() in the _evhtp_connection_readcb() func.

2. reason of the coredump:

for the default callback registration in the _evhtp_connection_accept() func, when receive a short connection request, the action flow of server is:

• at first come into _evhtp_connection_readcb() func and get the request data.
• then call htparser_run() to verify and parse the request data. in the htparser_run(), after completed parse msg, it will callback the func(for example,the func name is: handle_cmd) that registered by evhtp_set_cb().
• in the handle_cmd func, after process the request body, in common we will write the response data(for example, call evhtp_send_reply() func) in the end.
• then _evhtp_connection_writecb that registered in the _evhtp_connection_accept() will be triggered by the write action.
• and in the _evhtp_connection_writecb func, if the request is a short connection, it will call evhtp_connection_free() to free the connection.
• after return from htparser_run func, the connection variable in the _evhtp_connection_readcb func is become invalid. so call htparser_get_error func get a coredump.

3. code modify:

static int _evhtp_connection_accept(...) {
	...
    bufferevent_enable(connection->bev, EV_READ);
    bufferevent_setcb(connection->bev,
                      _evhtp_connection_readcb,
                      //_evhtp_connection_writecb,       <-- after write response back to the accepted connection, don't call _evhtp_connection_writecb()
                      NULL,
                      _evhtp_connection_eventcb, connection);	
	
	...
}

static void _evhtp_connection_readcb(...) {
	...
    evbuffer_drain(bufferevent_get_input(bev), nread);

    if (c->request && c->request->status == EVHTP_RES_PAUSE) {
        evhtp_request_pause(c->request);
    } else if (htparser_get_error(c->parser) != htparse_error_none) {
        evhtp_connection_free(c);
    } else if (nread < avail) {
        // we still have more data to read (piped request probably)
        evhtp_connection_resume(c);
    } else {					         <-- add this else to deal with the connection.
        if(c->type == evhtp_type_server) {
            /*
            * if there is a set maximum number of keepalive requests configured, check
            * to make sure we are not over it. If we have gone over the max we set the
            * keepalive bit to 0, thus closing the connection.
            */
            if (c->htp->max_keepalive_requests) {
                if (++c->num_requests >= c->htp->max_keepalive_requests) {
                    c->request->keepalive = 0;
                }
            }

            if (c->request->keepalive == 1) {
                _evhtp_request_free(c->request);

                c->keepalive       = 1;
                c->request         = NULL;
                c->body_bytes_read = 0;

                if (c->htp->parent && c->vhost_via_sni == 0) {
                    /* this request was servied by a virtual host evhtp_t structure
                    * which was *NOT* found via SSL SNI lookup. In this case we want to
                    * reset our connections evhtp_t structure back to the original so
                    * that subsequent requests can have a different Host: header.
                    */
                    evhtp_t * orig_htp = c->htp->parent;

                    c->htp = orig_htp;
                }

                htparser_init(c->parser, htp_type_request);
                htparser_set_userdata(c->parser, c);
            } else {
                evhtp_connection_free(c);
            }
        }
    }
	...	
}

i have merged above changes in my project, it works correct now.
above modification is suitable or not, can you give some suggestions. Thanks in advance.

[helinbo2015]

/sig bug

[NathanFrench]

Sweet! I just got done pushing a bunch of stuff tonight, I will check this awesomeness out tomorrow.

Thank you very much!

[NathanFrench]

Damn, I applaud your attention to detail. Working on it now.

[NathanFrench]

Just to be clear, evhtp_connection_writecb will be called once all data has successfully been written, not for each send. So removing it would cause issues for applications that need to know this. E.g., rate-limiting.

[NathanFrench]

What version are you using? Testing with the current pre-release, I cannot reproduce (using examples/test_basic.c):

curl -H'Connection: close' -vv -0 localhost:8081/simple/
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8081 (#0)
> GET /simple/ HTTP/1.0
> Host: localhost:8081
> User-Agent: curl/7.52.1
> Accept: */*
> Connection: close
> 
* HTTP 1.0, assume close after body
< HTTP/1.0 200 OK
< Content-Length: 6
< Content-Type: text/plain
< 
* Curl_http_done: called premature == 0
* Closing connection 0
simple                          

If possible, do you have a backtrace?

Edit:

I couldn't reproduce with the following versions:

1.2.6
1.2.7
1.2.8
1.2.9
1.2.10
1.2.11
1.2.11n
1.2.12-pre1
1.2.12-pre2
develop

And even so far back as 
1.1.6

[helinbo2015]

@NathanFrench thank you for your kindly reply. i have got code from [git clone ellzey/libevhtp] and i have checked the example test_basic.c, and it's no coredump problem.

and would you explain that how to judge all data has successfully been written?
it's not complete call evbuffer_add() and evhtp_send_reply() function?

Just to be clear, evhtp_connection_writecb will be called once all data has successfully been written, not >for each send. So removing it would cause issues for applications that need to know this. E.g., rate-limiting.

[helinbo2015]

add more info:
in my env, segment fault is happend.

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff4ef6700 (LWP 18934)]
0x0000000000412dd4 in htparser_get_error (p=0x0) at /root/workspace/cmd-agent-sdk-C/libevhtp/htparse.c:369
369 return p->error;
Missing separate debuginfos, use: debuginfo-install glibc-2.17-111.e1.x86_64 keyutils-libs-1.5.8-3.x86_64 krb
5-libs-1.13.2-12.h2.x86_64 libcom_err-1.42.9-7.x86_64 libevent-2.0.21-4.x86_64 libselinux-2.2.2-6.x86_64 openssl-libs-1.0.1e-51.7.h1.x86_64 pcre-8.32-15.1.x86_64 xz-libs-5.1.2-9alpha.x86_64 zlib-1.2.7-14.x86_64(gdb) info stack
#0 0x0000000000412dd4 in htparser_get_error (p=0x0)
at /root/workspace/cmd-agent-sdk-C/libevhtp/htparse.c:369
#1 0x000000000040d0d3 in _evhtp_connection_readcb (bev=0x7fffec004470, arg=0x7fffec001530)
at /root/workspace/cmd-agent-sdk-C/libevhtp/evhtp.c:2050
#2 0x00007ffff778d42d in consider_reading () from /lib64/libevent_openssl-2.0.so.5
#3 0x00007ffff778d451 in be_openssl_readeventcb () from /lib64/libevent_openssl-2.0.so.5
#4 0x00007ffff7ba3a14 in event_base_loop () from /lib64/libevent-2.0.so.5
#5 0x0000000000407df5 in create_s_server (arg=0x7fffffffe1d0)
at /root/workspace/cmd-agent-sdk-C/libevhtp/cmdagent/cmd_agent.c:729
#6 0x00007ffff6f20dc5 in start_thread () from /lib64/libpthread.so.0
#7 0x00007ffff684266d in clone () from /lib64/libc.so.6

the reason is that conn is already freed in the evhtp_connection_writecb func.

[helinbo2015]

in this web site, evhtp_connection_writecb is called when have data for writing.

writecb: callback to invoke when the file descriptor is ready for writing, or NULL if no callback is desired

and in the example test_basic.c, evhtp_connection_writecb is called after _evhtp_connection_readcb is completed. so coredump is not happened.

would you explain more clear the time of calling evhtp_connection_writecb?
Thanks in advance.

[NathanFrench]

Can you try the following:

evhtp_set_bev_flags(htp_context, BEV_OPT_DEFER_CALLBACKS|BEV_OPT_CLOSE_ON_FREE);

Also, have you tried on current releases?

[helinbo2015]

@NathanFrench
add the following setting after evhtp_new(), the coredump is disappeared. the write_cb callback is called after read_cb callback completed. thank you very much.

evhtp_set_bev_flags(htp_context, BEV_OPT_DEFER_CALLBACKS|BEV_OPT_CLOSE_ON_FREE);

[helinbo2015]

and i have tried the current release, if not add evhtp_set_bev_flags(htp_context, BEV_OPT_DEFER_CALLBACKS|BEV_OPT_CLOSE_ON_FREE);, the coredump also happened.
one more point is: http is no coredump problem, only https.

[helinbo2015]

and i think that BEV_OPT_DEFER_CALLBACKS is the workaround of this problem.
thank you once again.

[ikonopistsev]

you should set BEV_OPT_DEFER_CALLBACKS becouse you are using bufferevent_openssl_socket_new for https.

[NathanFrench]

you should set BEV_OPT_DEFER_CALLBACKS becouse you are using bufferevent_openssl_socket_new for https.

This bug has been fixed for ages and has been backpatched in Libevent 2.0.

But this is correct in another manner: you don't want events firing off while in mid-translation. DEFER_CALLBACKS is the solution.

[ikonopistsev]

may be it fixed, but i have the same problem with ssl (https) client, based on bufferevent (not evhtp_connection_new). i use libevent-2.1.7-rc.

[NathanFrench]

add the following setting after evhtp_new(), the coredump is disappeared. the write_cb callback is called after read_cb callback completed. thank you very much.

Sweet. May I close this ticket?

[helinbo2015]

@NathanFrench please close this issue. Thank you very much.