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

curl_multi_wait can set *numfds to -1 #707

Closed
jay opened this Issue Mar 8, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@jay
Member

jay commented Mar 8, 2016

I notice in the curl tool easy_transfer there is a check to see if curl_multi_wait set *numfds param to -1, however this is not mentioned in the documentation and in the user example there is no check. If a user had multiple handles and *numfds returned -1 would subsequent calls also return -1, and regardless I think this should be documented.

@bagder

This comment has been minimized.

Member

bagder commented Mar 9, 2016

I don't think we should allow it to return -1... To me, it seems like the only circumstance it does this is when Curl_poll returns a negative number (to signal error) but I don't think we should push that error back in that way. You agree?

@bagder

This comment has been minimized.

Member

bagder commented Mar 9, 2016

Here's what I have in mind. That first if() is already clearly wrong:

diff --git a/lib/multi.c b/lib/multi.c
index ad7d21f..1e6a8b0 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -905,11 +905,11 @@ CURLMcode curl_multi_wait(CURLM *multi_handle,
   if(nfds) {
     /* wait... */
     infof(data, "Curl_poll(%d ds, %d ms)\n", nfds, timeout_ms);
     i = Curl_poll(ufds, nfds, timeout_ms);

-    if(i) {
+    if(i > 0) {
       unsigned int j;
       /* copy revents results from the poll to the curl_multi_wait poll
          struct, the bit values of the actual underlying poll() implementation
          may not be the same as the ones in the public libcurl API! */
       for(j = 0; j < extra_nfds; j++) {
@@ -924,10 +924,12 @@ CURLMcode curl_multi_wait(CURLM *multi_handle,
           mask |= CURL_WAIT_POLLPRI;

         extra_fds[j].revents = mask;
       }
     }
+    else
+      i = 0;
   }
   else
     i = 0;

   free(ufds);
@jay

This comment has been minimized.

Member

jay commented Mar 9, 2016

That is what I'm unclear about, is the user supposed to do anything about that error, or would it be better to just perform and then does perform realize that happened and kick back a recv error for the handle. In any case we can't do i < 0 because i is unsigned there, so it would have to be (int)i which is implementation defined but I know is ok for certain compilers like gcc and msvc (and I know I've seen unsigned to signed of the same width elsewhere in the code so I guess it is pretty well defined in most other compilers when twos complement to do that). Or if not we use a separate int k and assign that to *ret

@bagder

This comment has been minimized.

Member

bagder commented Mar 9, 2016

I don't see what a user could do about such an error in this case so I don't think we do anyone any favors by returning -1 there.

The unsigned int point is great though, so my revised slightly larger take on this then looks like below. It does less reusing of the same variable for multiple purposes and uses a plain int properly for the Curl_poll() return value.

diff --git a/lib/multi.c b/lib/multi.c
index ad7d21f..d6f38c9 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -809,10 +809,11 @@ CURLMcode curl_multi_wait(CURLM *multi_handle,
   unsigned int i;
   unsigned int nfds = 0;
   unsigned int curlfds;
   struct pollfd *ufds = NULL;
   long timeout_internal;
+  int retcode = 0;

   if(!GOOD_MULTI_HANDLE(multi))
     return CURLM_BAD_HANDLE;

   /* If the internally desired timeout is actually shorter than requested from
@@ -901,16 +902,18 @@ CURLMcode curl_multi_wait(CURLM *multi_handle,
       ufds[nfds].events |= POLLOUT;
     ++nfds;
   }

   if(nfds) {
+    int pollrc;
     /* wait... */
-    infof(data, "Curl_poll(%d ds, %d ms)\n", nfds, timeout_ms);
-    i = Curl_poll(ufds, nfds, timeout_ms);
+    DEBUGF(infof(data, "Curl_poll(%d ds, %d ms)\n", nfds, timeout_ms));
+    pollrc = Curl_poll(ufds, nfds, timeout_ms);

-    if(i) {
+    if(pollrc > 0) {
       unsigned int j;
+      retcode = pollrc;
       /* copy revents results from the poll to the curl_multi_wait poll
          struct, the bit values of the actual underlying poll() implementation
          may not be the same as the ones in the public libcurl API! */
       for(j = 0; j < extra_nfds; j++) {
         unsigned short mask = 0;
@@ -925,16 +928,14 @@ CURLMcode curl_multi_wait(CURLM *multi_handle,

         extra_fds[j].revents = mask;
       }
     }
   }
-  else
-    i = 0;

   free(ufds);
   if(ret)
-    *ret = i;
+    *ret = retcode;
   return CURLM_OK;
 }

 /*
  * Curl_multi_connchanged() is called to tell that there is a connection in

@bagder bagder closed this in 77e1726 Mar 10, 2016

jay added a commit that referenced this issue Mar 19, 2016

easy: Remove poll failure check in easy_transfer
.. because curl_multi_wait can no longer signal poll failure.

follow-up to 77e1726

Bug: #707
@jay

This comment has been minimized.

Member

jay commented Mar 19, 2016

LGTM, I followed up in c574434 and removed the poll failure check in easy_transfer.

@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018

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