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

Jump to bottom

curl_multi_wait can set *numfds to -1 #707

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

curl_multi_wait can set *numfds to -1 #707

jay opened this issue Mar 8, 2016 · 5 comments

Comments

@jay
Copy link
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
Copy link
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
Copy link
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
Copy link
Member Author

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
Copy link
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 as completed in 77e1726 Mar 10, 2016
jay added a commit that referenced this issue Mar 19, 2016
@jay
easy: Remove poll failure check in easy_transfer
c574434 
.. because curl_multi_wait can no longer signal poll failure.

follow-up to 77e1726

Bug: #707
@jay
Copy link
Member Author

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.
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bagder @jay