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

7.81 Segmentation Fault #8282

Closed
updatede opened this issue Jan 16, 2022 · 6 comments
Closed

7.81 Segmentation Fault #8282

updatede opened this issue Jan 16, 2022 · 6 comments

Comments

@updatede
Copy link

@updatede updatede commented Jan 16, 2022

May application work good with 7.80, after upgrade from 7.80 to 7.81 I got segmentation fault.
After some trace, I found it's becuase the position of Curl_update_timer(multi) in curl_multi_add_handle changed.
When I make connection, I call curl_multi_add_handle, in it Curl_update_timer call my time_cb where I call curl_multi_socket_action, it call multi_socket,multi_runsingle,Curl_pretransfer, I use manually added dns resolve pair, so it call Curl_loadhostpairs, in Curl_loadhostpairs, it use data->dns.hostcache which is zero, so segmentation fault.
For 7.80, data->dns.hostcache is assigned before Curl_update_timer(multi) in curl_multi_add_handle, but for 7.81, data->dns.hostcache is assigned after Curl_update_timer(multi) because it's position changed. I move Curl_update_timer(multi) to the end of curl_multi_add_handle like 7.80, everthing is ok now.
Please put ``Curl_update_timer(multi)` in appropriate place in future version.

@bagder bagder added the crash label Jan 16, 2022
@bagder
Copy link
Member

@bagder bagder commented Jan 16, 2022

It would be very helpful if you could provide us with source code that reproduces your issue.

@updatede
Copy link
Author

@updatede updatede commented Jan 16, 2022

I write some simple code that can reproduces the issue.

#include <sys/socket.h>
#include <sys/types.h>


#include <arpa/inet.h>
#include <ctype.h>
#include <curl/curl.h>
#include <errno.h>
#include <ev.h>
#include <grp.h>
#include <netdb.h>
#include <netinet/in.h>
#include <pwd.h>
#include <signal.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <unistd.h>



static void sigint_cb(struct ev_loop *loop, ev_signal *w, int revents) {
  ev_break(loop, EVBREAK_ALL);
}

static void sigpipe_cb(struct ev_loop *loop, ev_signal *w, int revents) {
  return;
}

struct ev_loop *loop;
ev_timer timer;
ev_io fd;
CURLM *curlm;
int still_running;



static void sock_cb(struct ev_loop *loop, struct ev_io *w, int revents) {
  
 
  CURLMcode rc = curl_multi_socket_action(
      curlm, w->fd, (revents & EV_READ ? CURL_CSELECT_IN : 0) |
                       (revents & EV_WRITE ? CURL_CSELECT_OUT : 0),
      &still_running);
  if (rc != CURLM_OK) {
   return;
  }
  
}



static int multi_sock_cb(CURL *curl, curl_socket_t sock, int what,
                         void *c, void *sockp) {
  if (!curl) {
    return 0;
  }
 
  if (what == CURL_POLL_REMOVE) {
    ev_io_stop(loop, &fd);
    fd.fd = 0;
    return 0;
  } 
  if (fd.fd) {
    ev_io_stop(loop, &fd);
  }
  ev_io_init(&fd, sock_cb, sock,
             ((what & CURL_POLL_IN) ? EV_READ : 0) |
                 ((what & CURL_POLL_OUT) ? EV_WRITE : 0));

  ev_io_start(loop, &fd);
  return 0;
}

static void timer_cb(struct ev_loop *loop, struct ev_timer *w, int revents) {

  CURLMcode rc = curl_multi_socket_action(curlm, CURL_SOCKET_TIMEOUT, 0,
                                          &still_running);
  if (rc != CURLM_OK) {
    return;
  }
 
}

static int multi_timer_cb(CURLM *multi, long timeout_ms, void *c) {
  ev_timer_stop(loop, &timer);
  if (timeout_ms > 0) {
    ev_timer_init(&timer, timer_cb, timeout_ms / 1000.0, 0);
    ev_timer_start(loop, &timer);
  } else {
    timer_cb(loop, &timer, 0);
  }
  return 0;
}

static size_t write_buffer(void *buf, size_t size, size_t nmemb, void *userp) {
int len=size * nmemb;
   printf("%s",(char*)buf);
 
  return len;
}
int main(int argc, char *argv[]) {


  curl_global_init(CURL_GLOBAL_DEFAULT);

  
  	loop = EV_DEFAULT;
 	curlm= curl_multi_init();
	struct curl_slist *resolv=NULL;
	
	
	curl_multi_setopt(curlm, CURLMOPT_PIPELINING, CURLPIPE_MULTIPLEX);
	curl_multi_setopt(curlm, CURLMOPT_SOCKETFUNCTION, multi_sock_cb);
	curl_multi_setopt(curlm, CURLMOPT_TIMERFUNCTION, multi_timer_cb);
 
	resolv=curl_slist_append(resolv, "curl.se:443:151.101.130.49");	
	CURL *curl= curl_easy_init();
	CURLcode res;
	if ((res = curl_easy_setopt(curl, CURLOPT_RESOLVE, resolv)) !=
	CURLE_OK) {
		return 0;
	}
	
	curl_easy_setopt(curl, CURLOPT_HTTP_VERSION,CURL_HTTP_VERSION_2_0);
	curl_easy_setopt(curl, CURLOPT_URL, "https://curl.se");
	curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, &write_buffer);
	curl_easy_setopt(curl, CURLOPT_PIPEWAIT, 1L);
		
	curl_multi_add_handle(curlm, curl);
	
  ev_signal sigpipe;
  ev_signal_init(&sigpipe, sigpipe_cb, SIGPIPE);
  ev_signal_start(loop, &sigpipe);

  ev_signal sigint;
  ev_signal_init(&sigint, sigint_cb, SIGINT);
  ev_signal_start(loop, &sigint);

  ev_run(loop, 0);

   ev_signal_stop(loop, &sigint);

  

  ev_loop_destroy(loop);

  curl_global_cleanup();


  return EXIT_SUCCESS;
}

@bagder
Copy link
Member

@bagder bagder commented Jan 16, 2022

Here's a quote the man page for CURLMOPT_TIMERFUNCTION:

WARNING: even if it feels tempting, avoid calling libcurl directly from within the callback itself when the timeout_ms value is zero,

Yet, this is exactly what your callback does. If I remove that call, it doesn't seem to crash...

@updatede
Copy link
Author

@updatede updatede commented Jan 17, 2022

But there's no problem before 7.81. So I never think about re-read man pages, nevertheless, even the example of man page of CURLMOPT_TIMERFUNCTION call libcurl directly from within the CURLMOPT_TIMERFUNCTION callback,

static int timerfunc(CURLM *multi, long timeout_ms, void *userp)
{
  guint *id = userp;
 
  if(id)
    g_source_remove(*id);
 
  /* -1 means we should just delete our timer. */
  if(timeout_ms == -1) {
    g_free(id);
    id = NULL;
  }
  else {
    if(!id)
      id = g_new(guint, 1);
    *id = g_timeout_add(timeout_ms, timeout_cb, id);
  }
  curl_multi_setopt(multi, CURLMOPT_TIMERDATA, id);
  return 0;
}
 
curl_multi_setopt(multi, CURLMOPT_TIMERFUNCTION, timerfunc);

I think curl_multi_setopt(multi, CURLMOPT_TIMERDATA, id) must be libcurl. Only curl_multi_socket_action can't be called within callback ?
The whole thing is very confusing. Maybe there's better way to notify developer when make changes even not know who make mistake especially it will make your application crash.

@bagder
Copy link
Member

@bagder bagder commented Jan 17, 2022

But there's no problem before 7.81.

... that you encountered.

Recursively calling libcurl back with the same handles is very complicated and error-prone to handle. Mostly because of the local state that might have changed from underneath without notice when a callback returns.

Back in 2018 (b46cfbc) we made libcurl detect and refuse to get called back from callbacks, but due to how the code was written a bunch of callbacks didn't set this protection: we accidentally still allowed it from the multi handle callbacks. But we discouraged the use of it in the documentation since already before 2018.

Now I've proposed #8286 to prevent recursive function calls from these multi callbacks as well.

even the example of man page of CURLMOPT_TIMERFUNCTION

Yikes, that is bad. And wrong. I'll make sure those are corrected as well.

@bagder
Copy link
Member

@bagder bagder commented Jan 17, 2022

If you cannot make this crash happen without calling libcurl recursively, then I think we will just let it be and I would encourage you to change your code flow to not call libcurl recursively. In next release that function call will return an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants