[PATCH] bug in kqueue implementation of apr_pollset_poll (wrong conversion from usec)

[Marco Molteni]

Hi,

first of all, thanks for providing APR :-)

I think I've found a bug in the kqueue implementation of apr_pollset_poll().
I am using FreeBSD -current with apr-1.2.7, but the SVN trunk
version of kqueue.c has the same bug:


    232  APR_DECLARE(apr_status_t) apr_pollset_poll(apr_pollset_t *pollset,
    233                                             apr_interval_time_t timeout,
    234                                             apr_int32_t *num,
    235                                             const apr_pollfd_t 
**descriptors)
    236  {
    237      int ret, i;
    238      struct timespec tv, *tvptr;
    239      apr_status_t rv = APR_SUCCESS;
    240  
    241      if (timeout < 0) {
    242          tvptr = NULL;
    243      }
    244      else {
    245          tv.tv_sec = (long) apr_time_sec(timeout);
    246          tv.tv_nsec = (long) apr_time_msec(timeout);
    247          tvptr = &tv;
    248      }


lines 245,246 convert from timeout (usec) to struct timespec, but line 246
assigns milliseconds to nanoseconds, with a wrong magnitude of 10^6.

Net effect is that a timeout of say 999 msec becomes a timeout of 999 nsec.

Please find attached a proposed patch and a test program to expose the bug.

thanks
marco



--- kqueue.c.orig	Fri Aug 18 16:44:49 2006
+++ kqueue.c	Fri Aug 18 16:51:06 2006
@@ -240,13 +240,13 @@ APR_DECLARE(apr_status_t) apr_pollset_po
 
     if (timeout < 0) {
         tvptr = NULL;
     }
     else {
         tv.tv_sec = (long) apr_time_sec(timeout);
-        tv.tv_nsec = (long) apr_time_msec(timeout);
+        tv.tv_nsec = (long) apr_time_usec(timeout) * 1000;
         tvptr = &tv;
     }
 
     ret = kevent(pollset->kqueue_fd, NULL, 0, pollset->ke_set, pollset->nalloc,
                 tvptr);
     (*num) = ret;

/* apr_kqueue_bug.c
   Expose a bug in the kqueue version of apr_pollset_poll().

   compile with:
   cc -W -Wall -Werror -o apr_kqueue_bug apr_kqueue_bug.c \
       -I/usr/local/include/apr-1 -L /usr/local/lib -lapr-1
 */

#include <stdio.h>
#include <stdlib.h>
#include <apr_poll.h>

#define CHECK(x) do { \
    rv = x; \
    if (rv != APR_SUCCESS) { printf("%s %s", #x, errmsg(rv)); exit(1); } \
    } while (0)

const char *
errmsg(apr_status_t rv)
{
    static char buf[80];

    if (rv == APR_SUCCESS) {
        return "success";
    } else {
        apr_strerror(rv, buf, sizeof buf);
        return buf;
    }
}

void mywait(apr_pollset_t *pollset, apr_time_t timeout_us)
{
    apr_status_t rv;
    apr_time_t   start_us, stop_us;
    apr_pollfd_t const *ret_pfd;
    int          num_pfd = 0;

    start_us = apr_time_now();
    rv = apr_pollset_poll(pollset, timeout_us, &num_pfd, &ret_pfd);
    stop_us = apr_time_now();
    if (! APR_STATUS_IS_TIMEUP(rv)) {
        printf("unexpected apr_pollset_poll %s", errmsg(rv));
    }
    printf("wait: asked %lld ms, waited %lld ms\n",
        apr_time_as_msec(timeout_us), apr_time_as_msec(stop_us - start_us));
}

int main()
{
    apr_status_t   rv;
    apr_pollset_t *pollset;
    apr_pool_t    *pool;
    apr_pollfd_t   pfd;
    apr_socket_t  *sock;

    CHECK(apr_initialize());
    CHECK(apr_pool_create(&pool, NULL));
    CHECK(apr_pollset_create(&pollset, 512, pool, 0));
    CHECK(apr_socket_create(&sock, APR_INET, SOCK_STREAM, APR_PROTO_TCP, pool));

    pfd.desc_type = APR_POLL_SOCKET;
    pfd.reqevents = APR_POLLIN;
    pfd.desc.s = sock;
    CHECK(apr_pollset_add(pollset, &pfd));

    mywait(pollset,  999 * 1000); // 999 ms  => will wait 0 ms
    mywait(pollset, 1999 * 1000); // 1999 ms => will wait 1000 ms

    return 0;
}