Hi,

There is a tiny problem in 2.0.9 which prevents connects from working 
on FreeBSD (that's probably the same problem encountered by Zoong on 
OpenBSD...): the address length passed to connect should be the size 
of a sockaddr_in, not of the union of the various types of sockaddr_*s.

Hence the following patch against 2.0.9 that fixes the above (at line 
682 of http.c) + the inet_ntoa issue for the per-connexion IP 
addresses (not the backend error stuff). I haven't tested it in 
production yet, though, I'll do that tomorrow or Monday.

Jacques.

 > diff -u Pound-2.0.9.orig/http.c Pound-2.0.9/http.c
--- Pound-2.0.9.orig/http.c     Fri Jun 23 14:10:59 2006
+++ Pound-2.0.9/http.c  Sun Jul  2 03:23:55 2006
(at)(at) -401,8 +401,10 (at)(at)
      struct sockaddr_in  *srv;
      regmatch_t          matches[4];
      struct linger       l;
+    char               from_host_str[16];

      from_host = ((thr_arg *)arg)->from_host;
+    inet_ntop(AF_INET,&from_host,from_host_str,sizeof(from_host_str));
      lstn = ((thr_arg *)arg)->lstn;
      sock = ((thr_arg *)arg)->sock;
      free(arg);
(at)(at) -453,7 +455,7 (at)(at)
          cl = bb;
          if(BIO_do_handshake(cl) <= 0) {
              /* no need to log every client without a certificate...
-            logmsg(LOG_WARNING, "BIO_do_handshake with %s failed: 
%s", inet_ntoa(from_host),
+            logmsg(LOG_WARNING, "BIO_do_handshake with %s failed: 
%s", from_host_str,
                  ERR_error_string(ERR_get_error(), NULL));
              x509 = NULL;
              */
(at)(at) -463,7 +465,7 (at)(at)
          } else {
              if((x509 = SSL_get_peer_certificate(ssl)) != NULL && 
lstn->clnt_check < 3
              && SSL_get_verify_result(ssl) != X509_V_OK) {
-                logmsg(LOG_WARNING, "Bad certificate from %s", 
inet_ntoa(from_host));
+                logmsg(LOG_WARNING, "Bad certificate from %s", from_host_str);
                  BIO_reset(cl);
                  BIO_free_all(cl);
                  pthread_exit(NULL);
(at)(at) -494,7 +496,7 (at)(at)
          if((headers = get_headers(cl, cl, lstn)) == NULL) {
              if(!cl_11) {
                  if(errno)
-                    logmsg(LOG_WARNING, "error read from %s: %s", 
inet_ntoa(from_host), strerror(errno));
+                    logmsg(LOG_WARNING, "error read from %s: %s", 
from_host_str, strerror(errno));
                  /* err_reply(cl, h500, lstn->err500); */
              }
              clean_all();
(at)(at) -514,7 +516,7 (at)(at)
                      && strncasecmp(request + matches[1].rm_so, 
"DELETE", matches[1].rm_eo - matches[1].rm_so));
                      /* && strncasecmp(request + matches[1].rm_so, 
"OPTIONS", matches[1].rm_eo - matches[1].rm_so)); */
          } else {
-            logmsg(LOG_WARNING, "bad request \"%s\" from %s", 
request, inet_ntoa(from_host));
+            logmsg(LOG_WARNING, "bad request \"%s\" from %s", 
request, from_host_str);
              err_reply(cl, h501, lstn->err501);
              free_headers(headers);
              clean_all();
(at)(at) -524,7 +526,7 (at)(at)
          strncpy(url, request + matches[2].rm_so, matches[2].rm_eo - 
matches[2].rm_so);
          url[matches[2].rm_eo - matches[2].rm_so] = '\0';
          if(regexec(&lstn->url_pat,  url, 0, NULL, 0)) {
-            logmsg(LOG_WARNING, "bad URL \"%s\" from %s", url, 
inet_ntoa(from_host));
+            logmsg(LOG_WARNING, "bad URL \"%s\" from %s", url, from_host_str);
              err_reply(cl, h501, lstn->err501);
              free_headers(headers);
              clean_all();
(at)(at) -565,7 +567,7 (at)(at)
                  break;
              case HEADER_ILLEGAL:
                  if(log_level > 0)
-                    logmsg(LOG_WARNING, "bad header from %s (%s)", 
inet_ntoa(from_host), headers[n]);
+                    logmsg(LOG_WARNING, "bad header from %s (%s)", 
from_host_str, headers[n]);
                  headers_ok[n] = 0;
                  break;
              }
(at)(at) -609,7 +611,7 (at)(at)

          /* possibly limited request size */
          if(lstn->max_req > 0L && cont > 0L && cont
> lstn->max_req) {
-            logmsg(LOG_WARNING, "request too large (%ld) from %s", 
cont, inet_ntoa(from_host));
+            logmsg(LOG_WARNING, "request too large (%ld) from %s", 
cont, from_host_str);
              err_reply(cl, h501, lstn->err501);
              free_headers(headers);
              clean_all();
(at)(at) -627,14 +629,14 (at)(at)

          /* check that the requested URL still fits the old back-end 
(if any) */
          if((svc = get_service(lstn, url, &headers[1])) == NULL) {
-            logmsg(LOG_WARNING, "no service \"%s\" from %s", 
request, inet_ntoa(from_host));
+            logmsg(LOG_WARNING, "no service \"%s\" from %s", 
request, from_host_str);
              err_reply(cl, h503, lstn->err503);
              free_headers(headers);
              clean_all();
              pthread_exit(NULL);
          }
          if((backend = get_backend(svc, from_host, url, 
&headers[1])) == NULL) {
-            logmsg(LOG_WARNING, "no back-end \"%s\" from %s", 
request, inet_ntoa(from_host));
+            logmsg(LOG_WARNING, "no back-end \"%s\" from %s", 
request, from_host_str);
              err_reply(cl, h503, lstn->err503);
              free_headers(headers);
              clean_all();
(at)(at) -660,7 +662,7 (at)(at)
                      close(sock);
                      kill_be(svc, backend);
                      if((backend = get_backend(svc, from_host, url, 
&headers[1])) == NULL) {
-                        logmsg(LOG_WARNING, "no back-end \"%s\" from 
%s", request, inet_ntoa(from_host));
+                        logmsg(LOG_WARNING, "no back-end \"%s\" from 
%s", request, from_host_str);
                          err_reply(cl, h503, lstn->err503);
                          free_headers(headers);
                          clean_all();
(at)(at) -677,13 +679,13 (at)(at)
                      clean_all();
                      pthread_exit(NULL);
                  }
-                if(connect_nb(sock, (struct sockaddr 
*)&backend->addr.in, (socklen_t)sizeof(backend->addr),
backend->to) < 0) {
+                if(connect_nb(sock, (struct sockaddr 
*)&backend->addr.in, (socklen_t)sizeof(backend->addr.in),
backend->to) < 0) {
                      logmsg(LOG_WARNING, "backend %s:%hd connect: %s",
                          inet_ntoa(backend->addr.in.sin_addr), 
ntohs(backend->addr.in.sin_port), strerror(errno));
                      close(sock);
                      kill_be(svc, backend);
                      if((backend = get_backend(svc, from_host, url, 
&headers[1])) == NULL) {
-                        logmsg(LOG_WARNING, "no back-end \"%s\" from 
%s", request, inet_ntoa(from_host));
+                        logmsg(LOG_WARNING, "no back-end \"%s\" from 
%s", request, from_host_str);
                          err_reply(cl, h503, lstn->err503);
                          free_headers(headers);
                          clean_all();
(at)(at) -849,7 +851,7 (at)(at)
          }
          /* put additional client IP header */
          if(cur_backend->be_type == BACK_END) {
-            BIO_printf(be, "X-Forwarded-For: %s\r\n", inet_ntoa(from_host));
+            BIO_printf(be, "X-Forwarded-For: %s\r\n", from_host_str);

              /* final CRLF */
              BIO_puts(be, "\r\n");
(at)(at) -907,18 +909,18 (at)(at)
                  break;
              case 1:
              case 2:
-                logmsg(LOG_INFO, "%s %s - REDIRECT %s", 
inet_ntoa(from_host), request, cur_backend->url);
+                logmsg(LOG_INFO, "%s %s - REDIRECT %s", 
from_host_str, request, cur_backend->url);
                  break;
              case 3:
                  if(v_host[0])
-                    logmsg(LOG_INFO, "%s %s - %s [%s] \"%s\" 307 0 
\"%s\" \"%s\"", v_host, inet_ntoa(from_host),
+                    logmsg(LOG_INFO, "%s %s - %s [%s] \"%s\" 307 0 
\"%s\" \"%s\"", v_host, from_host_str,
                          u_name[0]? u_name: "-", 
log_time(req_start), request, referer, u_agent);
                  else
-                    logmsg(LOG_INFO, "%s - %s [%s] \"%s\" 307 0 
\"%s\" \"%s\"", inet_ntoa(from_host),
+                    logmsg(LOG_INFO, "%s - %s [%s] \"%s\" 307 0 
\"%s\" \"%s\"", from_host_str,
                          u_name[0]? u_name: "-", 
log_time(req_start), request, referer, u_agent);
                  break;
              case 4:
-                logmsg(LOG_INFO, "%s - %s [%s] \"%s\" 307 0 \"%s\" 
\"%s\"", inet_ntoa(from_host),
+                logmsg(LOG_INFO, "%s - %s [%s] \"%s\" 307 0 \"%s\" 
\"%s\"", from_host_str,
                      u_name[0]? u_name: "-", log_time(req_start), 
request, referer, u_agent);
                  break;
              }
(at)(at) -1000,7 +1002,7 (at)(at)
                  for(n = 0; n < MAXHEADERS && headers[n]; n++) {
                      if(BIO_printf(cl, "%s\r\n", headers[n]) <= 0) {
                          if(errno)
-                            logmsg(LOG_WARNING, "error write to %s: 
%s", inet_ntoa(from_host), strerror(errno));
+                            logmsg(LOG_WARNING, "error write to %s: 
%s", from_host_str, strerror(errno));
                          free_headers(headers);
                          clean_all();
                          pthread_exit(NULL);
(at)(at) -1013,7 +1015,7 (at)(at)
                  BIO_puts(cl, "\r\n");
              if(BIO_flush(cl) != 1) {
                  if(errno)
-                    logmsg(LOG_WARNING, "error flush headers to %s: 
%s", inet_ntoa(from_host), strerror(errno));
+                    logmsg(LOG_WARNING, "error flush headers to %s: 
%s", from_host_str, strerror(errno));
                  clean_all();
                  pthread_exit(NULL);
              }
(at)(at) -1091,7 +1093,7 (at)(at)
                  }
                  if(BIO_flush(cl) != 1) {
                      if(errno)
-                        logmsg(LOG_WARNING, "error final flush to 
%s: %s", inet_ntoa(from_host), strerror(errno));
+                        logmsg(LOG_WARNING, "error final flush to 
%s: %s", from_host_str, strerror(errno));
                      clean_all();
                      pthread_exit(NULL);
                  }
(at)(at) -1105,19 +1107,19 (at)(at)
          case 0:
              break;
          case 1:
-            logmsg(LOG_INFO, "%s %s - %s", inet_ntoa(from_host), 
request, response);
+            logmsg(LOG_INFO, "%s %s - %s", from_host_str, request, response);
              break;
          case 2:
              str_be(buf, MAXBUF - 1, cur_backend);
-            logmsg(LOG_INFO, "%s %s - %s (%s)", 
inet_ntoa(from_host), request, response, buf);
+            logmsg(LOG_INFO, "%s %s - %s (%s)", from_host_str, 
request, response, buf);
              break;
          case 3:
              logmsg(LOG_INFO, "%s %s - %s [%s] \"%s\" %c%c%c %s 
\"%s\" \"%s\"", v_host[0]? v_host: "-",
-                inet_ntoa(from_host), u_name[0]? u_name: "-", 
log_time(req_start), request, response[9],
+                from_host_str, u_name[0]? u_name: "-", 
log_time(req_start), request, response[9],
                  response[10], response[11], log_bytes(res_bytes), 
referer, u_agent);
              break;
          case 4:
-            logmsg(LOG_INFO, "%s - %s [%s] \"%s\" %c%c%c %s \"%s\" 
\"%s\"", inet_ntoa(from_host),
+            logmsg(LOG_INFO, "%s - %s [%s] \"%s\" %c%c%c %s \"%s\" 
\"%s\"", from_host_str,
                  u_name[0]? u_name: "-", log_time(req_start), 
request, response[9], response[10], response[11],
                  log_bytes(res_bytes), referer, u_agent);
              break;

On Sun, 2006-07-02 at 03:37 +0200, Jacques Caron wrote:[...]

Many thanks for finding that bug - it will be fixed in the next release.

As to inet_ntoa issue - we're doing it slightly differently, as I happen
to be worried about the availability of inet_ntop on all systems. Again,
in the next release.[...]

Hi,

At 19:33 03/07/2006, Robert Segall wrote:[...]

I wondered about it, but I don't have much recent experience with 
other systems so I did not know if it was something commonly found or 
not. I guess a good old *printf("%d.%d.%d.%d") would do :-)

The next question probably is: are there other functions used that 
may not be thread-safe? I'm not a big fan of threads so I'm not 
totally aware of the common issues, but I believe localtime and 
gethostbyname may be a problem at least on some systems/versions 
(gethostbyname is only thread-safe in recent versions of FreeBSD, for 
instance).

At the very least, these internal functions using a static buffer are 
a problem:

static char *
log_time(time_t when)
{
     static char res[32];

     strftime(res, sizeof(res), "%d/%b/%Y:%H:%M:%S %z", localtime(&when));
     return res;
}

/*
  * Apache log-file-style number format
  */
static char *
log_bytes(long cnt)
{
     static char res[16];

     if(cnt > 0L)
         snprintf(res, sizeof(res), "%ld", cnt);
     else
         strcpy(res, "-");
     return res;
}

Hope that helps,

Jacques.

On Mon, Jul 03, 2006 at 07:33:41PM +0200, Robert Segall wrote:[...]

Actually, using inet_ntop() and eventually providing a fake
implementation when run on an ancient system would be a lot better,
at least if IPv6 is going to be supported.

I already migrated my servers to IPv6, and currently use nasty hacks
involving 6tunnel on both sides of pound.  Not a pretty sight, and
yet it makes firewalling a lot simpler (and simple = secure).  I
really would like to go away from this atrocity; a glance at the code
suggests that upgrading the listening side of pound should be very
straightforward with the logging part (and inet_ntop!) being a good
part of the effort.

The backend part currently involves protocol family related hacks,
and would be more tricky; but migrating the separate code for each
protocol family (PF_INET and PF_UNIX) to proper pf-agnostic code
would have a side effect of simplyfing things.  And simple = secure,
again.


Regards,[...]