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

disable wildcards for non-FTP protocols to fix timeouts #2016

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@cmeister2
Contributor

cmeister2 commented Oct 25, 2017

Fixes curl/curl-fuzzer#9.

Here's my proposal for a fix for more infinite loop fun caused by CURLOPT_WILDCARDMATCH. As ftp is the only protocol that advances the state of data->wildcard.state, we add in a special entry in DO_DONE where we set the state to CURLWC_DONE for non-FTP protocols (as we definitely have a connection in handle and thus the handler information).

Would appreciate alternative solutions.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 25, 2017

Member

A great first take!

My suggestion is that we instead of doing that multi.c-hack, move the wildcardmatch field to the state section of the easy handle (initied in the Curl_init_do function), and we switch it off if the protocol in use doesn't support wildcards. Like this:

From 16b36a6cd81d99f3d513af574a3a814c569a6515 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Thu, 26 Oct 2017 00:41:05 +0200
Subject: [PATCH] url: switch off wildcard use for protocols not supporting it

---
 lib/ftp.c      |  4 ++--
 lib/multi.c    |  6 +++---
 lib/transfer.c |  3 ++-
 lib/url.c      | 17 +++++++++--------
 lib/urldata.h  |  4 ++--
 5 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/lib/ftp.c b/lib/ftp.c
index 72ab24b5b..5c7df2b4c 100644
--- a/lib/ftp.c
+++ b/lib/ftp.c
@@ -3176,11 +3176,11 @@ static CURLcode ftp_done(struct connectdata *conn, CURLcode status,
   }
 
   /* now store a copy of the directory we are in */
   free(ftpc->prevpath);
 
-  if(data->set.wildcardmatch) {
+  if(data->state.wildcardmatch) {
     if(data->set.chunk_end && ftpc->file) {
       data->set.chunk_end(data->wildcard.customptr);
     }
     ftpc->known_filesize = -1;
   }
@@ -3961,11 +3961,11 @@ static CURLcode ftp_do(struct connectdata *conn, bool *done)
   struct ftp_conn *ftpc = &conn->proto.ftpc;
 
   *done = FALSE; /* default to false */
   ftpc->wait_data_conn = FALSE; /* default to no such wait */
 
-  if(conn->data->set.wildcardmatch) {
+  if(conn->data->state.wildcardmatch) {
     result = wc_statemach(conn);
     if(conn->data->wildcard.state == CURLWC_SKIP ||
       conn->data->wildcard.state == CURLWC_DONE) {
       /* do not call ftp_regular_transfer */
       return CURLE_OK;
diff --git a/lib/multi.c b/lib/multi.c
index ab2681bcf..658a18b15 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -1661,11 +1661,11 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
         /* When multi_do() returns failure, data->easy_conn might be NULL! */
 
         if(!result) {
           if(!dophase_done) {
             /* some steps needed for wildcard matching */
-            if(data->set.wildcardmatch) {
+            if(data->state.wildcardmatch) {
               struct WildcardData *wc = &data->wildcard;
               if(wc->state == CURLWC_DONE || wc->state == CURLWC_SKIP) {
                 /* skip some states if it is important */
                 multi_done(&data->easy_conn, CURLE_OK, FALSE);
                 multistate(data, CURLM_STATE_DONE);
@@ -1814,11 +1814,11 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
       if((data->easy_conn->sockfd != CURL_SOCKET_BAD) ||
          (data->easy_conn->writesockfd != CURL_SOCKET_BAD))
         multistate(data, CURLM_STATE_WAITPERFORM);
       else
       {
-        if(data->set.wildcardmatch &&
+        if(data->state.wildcardmatch &&
            ((data->easy_conn->handler->flags & PROTOPT_WILDCARD) == 0)) {
            data->wildcard.state = CURLWC_DONE;
         }
         multistate(data, CURLM_STATE_DONE);
       }
@@ -2036,11 +2036,11 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
          */
         if(data->easy_conn)
           data->easy_conn = NULL;
       }
 
-      if(data->set.wildcardmatch) {
+      if(data->state.wildcardmatch) {
         if(data->wildcard.state != CURLWC_DONE) {
           /* if a wildcard is set and we are not ending -> lets start again
              with CURLM_STATE_INIT */
           multistate(data, CURLM_STATE_INIT);
           break;
diff --git a/lib/transfer.c b/lib/transfer.c
index c4cc16060..75659cf44 100644
--- a/lib/transfer.c
+++ b/lib/transfer.c
@@ -1342,10 +1342,11 @@ CURLcode Curl_pretransfer(struct Curl_easy *data)
      before any transfer takes place. */
   result = Curl_ssl_initsessions(data, data->set.general_ssl.max_ssl_sessions);
   if(result)
     return result;
 
+  data->state.wildcardmatch = data->set.wildcard_enabled;
   data->set.followlocation = 0; /* reset the location-follow counter */
   data->state.this_is_a_follow = FALSE; /* reset this */
   data->state.errorbuf = FALSE; /* no error has occurred */
   data->state.httpversion = 0; /* don't assume any particular server version */
 
@@ -1399,11 +1400,11 @@ CURLcode Curl_pretransfer(struct Curl_easy *data)
        in the session we need to make sure we only use the one(s) we now
        consider to be fine */
     data->state.authhost.picked &= data->state.authhost.want;
     data->state.authproxy.picked &= data->state.authproxy.want;
 
-    if(data->set.wildcardmatch) {
+    if(data->state.wildcardmatch) {
       struct WildcardData *wc = &data->wildcard;
       if(wc->state < CURLWC_INIT) {
         result = Curl_wildcard_init(wc); /* init wildcard structures */
         if(result)
           return CURLE_OUT_OF_MEMORY;
diff --git a/lib/url.c b/lib/url.c
index 3a913dae8..f79380945 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -487,16 +487,12 @@ CURLcode Curl_close(struct Curl_easy *data)
     Curl_share_lock(data, CURL_LOCK_DATA_SHARE, CURL_LOCK_ACCESS_SINGLE);
     data->share->dirty--;
     Curl_share_unlock(data, CURL_LOCK_DATA_SHARE);
   }
 
-  if(data->set.wildcardmatch) {
-    /* destruct wildcard structures if it is needed */
-    struct WildcardData *wc = &data->wildcard;
-    Curl_wildcard_dtor(wc);
-  }
-
+  /* destruct wildcard structures if it is needed */
+  Curl_wildcard_dtor(&data->wildcard);
   Curl_freeset(data);
   free(data);
   return CURLE_OK;
 }
 
@@ -607,11 +603,11 @@ CURLcode Curl_init_userdefined(struct UserDefined *set)
   result = setstropt(&set->str[STRING_SSL_CAPATH_PROXY], CURL_CA_PATH);
   if(result)
     return result;
 #endif
 
-  set->wildcardmatch  = FALSE;
+  set->wildcard_enabled = FALSE;
   set->chunk_bgn      = ZERO_NULL;
   set->chunk_end      = ZERO_NULL;
 
   /* tcp keepalives are disabled by default, but provide reasonable values for
    * the interval and idle times.
@@ -2964,11 +2960,11 @@ CURLcode Curl_setopt(struct Curl_easy *data, CURLoption option,
     /* Set the user defined RTP write function */
     data->set.fwrite_rtp = va_arg(param, curl_write_callback);
     break;
 
   case CURLOPT_WILDCARDMATCH:
-    data->set.wildcardmatch = (0 != va_arg(param, long)) ? TRUE : FALSE;
+    data->set.wildcard_enabled = (0 != va_arg(param, long)) ? TRUE : FALSE;
     break;
   case CURLOPT_CHUNK_BGN_FUNCTION:
     data->set.chunk_bgn = va_arg(param, curl_chunk_bgn_callback);
     break;
   case CURLOPT_CHUNK_END_FUNCTION:
@@ -7223,10 +7219,15 @@ CURLcode Curl_init_do(struct Curl_easy *data, struct connectdata *conn)
                                  * use */
 
   data->state.done = FALSE; /* *_done() is not called yet */
   data->state.expect100header = FALSE;
 
+  /* if the protocol used doesn't support wildcards, switch it off */
+  if(data->state.wildcardmatch &&
+     !(conn->handler->flags & PROTOPT_WILDCARD))
+    data->state.wildcardmatch = FALSE;
+
   if(data->set.opt_no_body)
     /* in HTTP lingo, no body means using the HEAD request... */
     data->set.httpreq = HTTPREQ_HEAD;
   else if(HTTPREQ_HEAD == data->set.httpreq)
     /* ... but if unset there really is no perfect method that is the
diff --git a/lib/urldata.h b/lib/urldata.h
index 6b6c471d8..30e13a452 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -1307,11 +1307,11 @@ struct UrlState {
   /* a place to store the most recently set FTP entrypath */
   char *most_recent_ftp_entrypath;
 
   /* set after initial USER failure, to prevent an authentication loop */
   bool ftp_trying_alternative;
-
+  bool wildcardmatch; /* enable wildcard matching */
   int httpversion;       /* the lowest HTTP version*10 reported by any server
                             involved in this request */
   bool expect100header;  /* TRUE if we added Expect: 100-continue */
 
   bool pipe_broke; /* TRUE if the connection we were pipelined on broke
@@ -1671,11 +1671,11 @@ struct UserDefined {
   struct curl_slist *mail_rcpt; /* linked list of mail recipients */
   bool sasl_ir;         /* Enable/disable SASL initial response */
   /* Common RTSP header options */
   Curl_RtspReq rtspreq; /* RTSP request type */
   long rtspversion; /* like httpversion, for RTSP */
-  bool wildcardmatch; /* enable wildcard matching */
+  bool wildcard_enabled; /* enable wildcard matching */
   curl_chunk_bgn_callback chunk_bgn; /* called before part of transfer
                                         starts */
   curl_chunk_end_callback chunk_end; /* called after part transferring
                                         stopped */
   curl_fnmatch_callback fnmatch; /* callback to decide which file corresponds
-- 
2.15.0.rc1
Member

bagder commented Oct 25, 2017

A great first take!

My suggestion is that we instead of doing that multi.c-hack, move the wildcardmatch field to the state section of the easy handle (initied in the Curl_init_do function), and we switch it off if the protocol in use doesn't support wildcards. Like this:

From 16b36a6cd81d99f3d513af574a3a814c569a6515 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Thu, 26 Oct 2017 00:41:05 +0200
Subject: [PATCH] url: switch off wildcard use for protocols not supporting it

---
 lib/ftp.c      |  4 ++--
 lib/multi.c    |  6 +++---
 lib/transfer.c |  3 ++-
 lib/url.c      | 17 +++++++++--------
 lib/urldata.h  |  4 ++--
 5 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/lib/ftp.c b/lib/ftp.c
index 72ab24b5b..5c7df2b4c 100644
--- a/lib/ftp.c
+++ b/lib/ftp.c
@@ -3176,11 +3176,11 @@ static CURLcode ftp_done(struct connectdata *conn, CURLcode status,
   }
 
   /* now store a copy of the directory we are in */
   free(ftpc->prevpath);
 
-  if(data->set.wildcardmatch) {
+  if(data->state.wildcardmatch) {
     if(data->set.chunk_end && ftpc->file) {
       data->set.chunk_end(data->wildcard.customptr);
     }
     ftpc->known_filesize = -1;
   }
@@ -3961,11 +3961,11 @@ static CURLcode ftp_do(struct connectdata *conn, bool *done)
   struct ftp_conn *ftpc = &conn->proto.ftpc;
 
   *done = FALSE; /* default to false */
   ftpc->wait_data_conn = FALSE; /* default to no such wait */
 
-  if(conn->data->set.wildcardmatch) {
+  if(conn->data->state.wildcardmatch) {
     result = wc_statemach(conn);
     if(conn->data->wildcard.state == CURLWC_SKIP ||
       conn->data->wildcard.state == CURLWC_DONE) {
       /* do not call ftp_regular_transfer */
       return CURLE_OK;
diff --git a/lib/multi.c b/lib/multi.c
index ab2681bcf..658a18b15 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -1661,11 +1661,11 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
         /* When multi_do() returns failure, data->easy_conn might be NULL! */
 
         if(!result) {
           if(!dophase_done) {
             /* some steps needed for wildcard matching */
-            if(data->set.wildcardmatch) {
+            if(data->state.wildcardmatch) {
               struct WildcardData *wc = &data->wildcard;
               if(wc->state == CURLWC_DONE || wc->state == CURLWC_SKIP) {
                 /* skip some states if it is important */
                 multi_done(&data->easy_conn, CURLE_OK, FALSE);
                 multistate(data, CURLM_STATE_DONE);
@@ -1814,11 +1814,11 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
       if((data->easy_conn->sockfd != CURL_SOCKET_BAD) ||
          (data->easy_conn->writesockfd != CURL_SOCKET_BAD))
         multistate(data, CURLM_STATE_WAITPERFORM);
       else
       {
-        if(data->set.wildcardmatch &&
+        if(data->state.wildcardmatch &&
            ((data->easy_conn->handler->flags & PROTOPT_WILDCARD) == 0)) {
            data->wildcard.state = CURLWC_DONE;
         }
         multistate(data, CURLM_STATE_DONE);
       }
@@ -2036,11 +2036,11 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
          */
         if(data->easy_conn)
           data->easy_conn = NULL;
       }
 
-      if(data->set.wildcardmatch) {
+      if(data->state.wildcardmatch) {
         if(data->wildcard.state != CURLWC_DONE) {
           /* if a wildcard is set and we are not ending -> lets start again
              with CURLM_STATE_INIT */
           multistate(data, CURLM_STATE_INIT);
           break;
diff --git a/lib/transfer.c b/lib/transfer.c
index c4cc16060..75659cf44 100644
--- a/lib/transfer.c
+++ b/lib/transfer.c
@@ -1342,10 +1342,11 @@ CURLcode Curl_pretransfer(struct Curl_easy *data)
      before any transfer takes place. */
   result = Curl_ssl_initsessions(data, data->set.general_ssl.max_ssl_sessions);
   if(result)
     return result;
 
+  data->state.wildcardmatch = data->set.wildcard_enabled;
   data->set.followlocation = 0; /* reset the location-follow counter */
   data->state.this_is_a_follow = FALSE; /* reset this */
   data->state.errorbuf = FALSE; /* no error has occurred */
   data->state.httpversion = 0; /* don't assume any particular server version */
 
@@ -1399,11 +1400,11 @@ CURLcode Curl_pretransfer(struct Curl_easy *data)
        in the session we need to make sure we only use the one(s) we now
        consider to be fine */
     data->state.authhost.picked &= data->state.authhost.want;
     data->state.authproxy.picked &= data->state.authproxy.want;
 
-    if(data->set.wildcardmatch) {
+    if(data->state.wildcardmatch) {
       struct WildcardData *wc = &data->wildcard;
       if(wc->state < CURLWC_INIT) {
         result = Curl_wildcard_init(wc); /* init wildcard structures */
         if(result)
           return CURLE_OUT_OF_MEMORY;
diff --git a/lib/url.c b/lib/url.c
index 3a913dae8..f79380945 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -487,16 +487,12 @@ CURLcode Curl_close(struct Curl_easy *data)
     Curl_share_lock(data, CURL_LOCK_DATA_SHARE, CURL_LOCK_ACCESS_SINGLE);
     data->share->dirty--;
     Curl_share_unlock(data, CURL_LOCK_DATA_SHARE);
   }
 
-  if(data->set.wildcardmatch) {
-    /* destruct wildcard structures if it is needed */
-    struct WildcardData *wc = &data->wildcard;
-    Curl_wildcard_dtor(wc);
-  }
-
+  /* destruct wildcard structures if it is needed */
+  Curl_wildcard_dtor(&data->wildcard);
   Curl_freeset(data);
   free(data);
   return CURLE_OK;
 }
 
@@ -607,11 +603,11 @@ CURLcode Curl_init_userdefined(struct UserDefined *set)
   result = setstropt(&set->str[STRING_SSL_CAPATH_PROXY], CURL_CA_PATH);
   if(result)
     return result;
 #endif
 
-  set->wildcardmatch  = FALSE;
+  set->wildcard_enabled = FALSE;
   set->chunk_bgn      = ZERO_NULL;
   set->chunk_end      = ZERO_NULL;
 
   /* tcp keepalives are disabled by default, but provide reasonable values for
    * the interval and idle times.
@@ -2964,11 +2960,11 @@ CURLcode Curl_setopt(struct Curl_easy *data, CURLoption option,
     /* Set the user defined RTP write function */
     data->set.fwrite_rtp = va_arg(param, curl_write_callback);
     break;
 
   case CURLOPT_WILDCARDMATCH:
-    data->set.wildcardmatch = (0 != va_arg(param, long)) ? TRUE : FALSE;
+    data->set.wildcard_enabled = (0 != va_arg(param, long)) ? TRUE : FALSE;
     break;
   case CURLOPT_CHUNK_BGN_FUNCTION:
     data->set.chunk_bgn = va_arg(param, curl_chunk_bgn_callback);
     break;
   case CURLOPT_CHUNK_END_FUNCTION:
@@ -7223,10 +7219,15 @@ CURLcode Curl_init_do(struct Curl_easy *data, struct connectdata *conn)
                                  * use */
 
   data->state.done = FALSE; /* *_done() is not called yet */
   data->state.expect100header = FALSE;
 
+  /* if the protocol used doesn't support wildcards, switch it off */
+  if(data->state.wildcardmatch &&
+     !(conn->handler->flags & PROTOPT_WILDCARD))
+    data->state.wildcardmatch = FALSE;
+
   if(data->set.opt_no_body)
     /* in HTTP lingo, no body means using the HEAD request... */
     data->set.httpreq = HTTPREQ_HEAD;
   else if(HTTPREQ_HEAD == data->set.httpreq)
     /* ... but if unset there really is no perfect method that is the
diff --git a/lib/urldata.h b/lib/urldata.h
index 6b6c471d8..30e13a452 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -1307,11 +1307,11 @@ struct UrlState {
   /* a place to store the most recently set FTP entrypath */
   char *most_recent_ftp_entrypath;
 
   /* set after initial USER failure, to prevent an authentication loop */
   bool ftp_trying_alternative;
-
+  bool wildcardmatch; /* enable wildcard matching */
   int httpversion;       /* the lowest HTTP version*10 reported by any server
                             involved in this request */
   bool expect100header;  /* TRUE if we added Expect: 100-continue */
 
   bool pipe_broke; /* TRUE if the connection we were pipelined on broke
@@ -1671,11 +1671,11 @@ struct UserDefined {
   struct curl_slist *mail_rcpt; /* linked list of mail recipients */
   bool sasl_ir;         /* Enable/disable SASL initial response */
   /* Common RTSP header options */
   Curl_RtspReq rtspreq; /* RTSP request type */
   long rtspversion; /* like httpversion, for RTSP */
-  bool wildcardmatch; /* enable wildcard matching */
+  bool wildcard_enabled; /* enable wildcard matching */
   curl_chunk_bgn_callback chunk_bgn; /* called before part of transfer
                                         starts */
   curl_chunk_end_callback chunk_end; /* called after part transferring
                                         stopped */
   curl_fnmatch_callback fnmatch; /* callback to decide which file corresponds
-- 
2.15.0.rc1

@bagder bagder added the FTP label Oct 25, 2017

@cmeister2

This comment has been minimized.

Show comment
Hide comment
@cmeister2

cmeister2 Oct 25, 2017

Contributor

Yep, perfectly happy with that approach. Let me know if you want me to apply that locally or whether you'll handle this separately.

Contributor

cmeister2 commented Oct 25, 2017

Yep, perfectly happy with that approach. Let me know if you want me to apply that locally or whether you'll handle this separately.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 26, 2017

Member

Maybe you can apply that in your branch? This way we'll get the CI tests make sure it doesn't break anything before I consider merging it (and saves me from doing a parallel PR for the same problem).

Member

bagder commented Oct 26, 2017

Maybe you can apply that in your branch? This way we'll get the CI tests make sure it doesn't break anything before I consider merging it (and saves me from doing a parallel PR for the same problem).

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder
Member

bagder commented Oct 26, 2017

@bagder bagder changed the title from wip: Propose solution for wildcard timeouts to disable wildcards for non-FTP protocols to fix timeouts Oct 26, 2017

@cmeister2

This comment has been minimized.

Show comment
Hide comment
@cmeister2

cmeister2 Oct 26, 2017

Contributor

Awesome, thanks. I've applied it, should be going through CI now.

Contributor

cmeister2 commented Oct 26, 2017

Awesome, thanks. I've applied it, should be going through CI now.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 26, 2017

Member

Thanks!

Member

bagder commented Oct 26, 2017

Thanks!

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