Re: POP3d locking

[Sergey Poznyakoff]

Hello,

Sorry for being late with the answer.

> 
> > The select() or read() calls in pop3d_readline() can now be
> > interrupted by SIGALRM, so appropriate handling for EINTR is
> > added to the function.
> 
> This is a problem. In the case of the read() and accept() we can
> control it by checking EINTR.  But if the alarm goes off when doing
> a write, in the middle of RETR or TOP, we have a situation.
> 
> To send the data we use stdio fprintf (ofile, ..).  Stdio does not provide
> a clean way to recuperate or to know about an interruption.  Meaning
> deep inside stdio calls the write() will fail and in theory the
> error will propagate up and fprintf () will return a smaller number
> then ask and its up to us to call ferror().  In the end, we'll have
> to check for every single fprintf (ofile, ...) calls and contraily
> to the read()/accept() which are at one place fprintf() is everywhere No fun.
> 
> So I left out this portion of the patch.
Sure. You're right. I didn't think about it. But then we need to call
pop3d_touchlock() right after reading a next line, like that:

Index: pop3d/pop3d.c
===================================================================
RCS file: /cvs/mailutils/pop3d/pop3d.c,v
retrieving revision 1.27
diff -c -b -w -r1.27 pop3d.c
*** pop3d/pop3d.c       2001/05/02 04:45:12     1.27
--- pop3d/pop3d.c       2001/05/03 10:29:58
***************
*** 303,308 ****
--- 304,311 ----
            pop3d_abquit (ERR_MBOX_SYNC); /* Out of sync, Bail out.  */
        }
  
+       pop3d_touchlock ();
+       
        if (strlen (arg) > POP_MAXCMDLEN || strlen (cmd) > POP_MAXCMDLEN)
        status = ERR_TOO_LONG;
        else if (strlen (cmd) > 4)


Thus we could keep the lock 'warm' all the time. What do you think?

> > 3. Current implementation of pop3d_signal would cause main process
> >    to attempt to perform fprintf on NULL file pointer (ofile). Sure,
> >    the main process should not crash, but one never knows... So I
> >    have modified pop3d_signal to handle this appropriately.
> 
> Do not see why:
> ===== pop3d.c ======
> int
> pop3d_mainloop (int infile, int outfile)
> {
>   int status = OK;
> 
>   ifile = infile;
>   ofile = fdopen (outfile, "w");
>   if (ofile == NULL)
>     pop3d_abquit (ERR_NO_OFILE);
> ...
> }
> 
> This pretty much guaranty that ofile will not be NULL and if it is
> the server will immediately exit.
Not quite so, I believe. This guarranty that ofile will not be NULL in
the *child* process:

      pid = fork ();
      if (pid == -1)
        syslog(LOG_ERR, "fork: %s", strerror (errno));
      else if (pid == 0) /* Child.  */
        {
          int status;
          close (listenfd);
          status = pop3d_mainloop (connfd, connfd);

whereas in the parent one, ofile will always be NULL, since it lives
in the BSS (FILE *ofile;) and gets initialized only in the child
process. Thus, when the parent receives any signal, SIGTERM for
example, it does pop3d_abquit(ERR_SIGNAL) and dies doing
fprintf(ofile, ...


> Questons:
> - Is PIPELINING widely use in pop3d clients?
I don't suppose so. Anyway, I believe it is a good idea to implement
it. But I would rather propose to use streams instead of trying
to do that manually, i.e.:

in pop3d_mainloop():

        FILE *fp;

        fp = fdopen(ifile, "r");


then, in pop3d_readline, we could just do fgets(buf, sizeof(buf), fp)
without worrying about buffering stuff. We will not be able, however,
to use select(), but a signal handler for SIGALRM will do the work
perfectly. The pop3d_readline() could be like that:

static volatile int got_alrm;

RETSIGTYPE
sig_alrm(int signo)
{
  got_alrm = 1;
}

char *
pop3d_readline(FILE *fp)
{
  static char buf[512];
  if (timeout)
    {
      signal(SIGALRM, sig_alrm);
      alarm(timeout);
    }
  ptr = fgets(buf, sizeof(buf), fp);
  if (timeout)
    signal(SIGALRM, SIG_DFL);
  if (!got_alrm)
    pop3d_abquit (ERR_TIMEOUT);
  if (!ptr && ferror(fp))
    pop3d_abquit(ERR_NO_FILE);  
  return ptr;
}


What do you think?

Besides, there is a problem in the currently proposed implementation
of pop3d_readline():

at the first invocation of the function memchr(buffer, '\n', total)
is called upon NULL pointer (buffer = NULL at the start of the
fuction). This makes program coredump on Solaris (and possibly
on other SYSV-like systems).

Maybe this would be better:

char *
pop3d_readline (int fd)
{
  static char *buffer = NULL; /* Note: This buffer is never free()d.  */
  static size_t total = 0;
  char *nl;
  char *line;
  size_t len;

  while ((nl = (buffer ? memchr (buffer, '\n', total) : NULL)) == NULL)
    {
      char buf[512];
      int nread;

      if (timeout)
        {
          int available;
          fd_set rfds;
          struct timeval tv;
          
          FD_ZERO (&rfds);
          FD_SET (fd, &rfds);
          tv.tv_sec = timeout;
          tv.tv_usec = 0;
          
          available = select (fd + 1, &rfds, NULL, NULL, &tv);
          if (!available)
            pop3d_abquit (ERR_TIMEOUT);
          else if (available == -1)
            {
              if (errno == EINTR)
                continue;
              pop3d_abquit (ERR_NO_OFILE);
            }
        }

      errno = 0;
      nread = read (fd, buf, sizeof (buf) - 1);
      if (nread < 1)
        {
          if (errno == EINTR)
            continue;
          pop3d_abquit (ERR_NO_OFILE);
        }
      buf[nread] = '\0';
      
      buffer = realloc (buffer, (total + nread + 1) * sizeof (*buffer));
      if (buffer == NULL)
        pop3d_abquit (ERR_NO_MEM);
      memcpy (buffer + total, buf, nread + 1); /* copy the null too.  */
      total += nread;
    }

  nl++;
  len = nl - buffer;
  line = calloc (len + 1, sizeof (*line));
  memcpy (line, buffer, len); /* copy the newline too.  */
  line[len] = '\0';
  total -= len;
  memmove (buffer, nl, total);
  return line;
}


Another question: the removal of closelog(), openlog() from the pam part
of user.c seems to cause problems. Unfortunately most of the pam modules
do openlog() on their own, thus after authentication all logging
output flows to where the last pam module has directed it, which
is usually `auth' facility. May be we would better keep the closelog()
openlog() pair? or at least make it somehow configuration-dependent?

Cheers,
Sergey Poznyakoff

------------------------------------------------------------------------------
... on n'est jamais heureux la ou l'on est ...