Re: Bug in POP3 code

[Gordon Oliver <gordo pincoya com>]

On 2001.10.05 07:54 Emmanuel wrote:
> static PopStatus
> fetch_pop_mail (const gchar *pop_host, const gchar *pop_user,
> 		const gchar *pop_pass, gint pop_port,
> 		gboolean use_apop, gboolean delete_on_server,
> 		gchar * last_uid, ProgressCallback prog_cb,
> 		ProcessCallback proccb, const gchar *data)
> {
>     char buffer[2048];
>     char uid[80];
>     int s, msgs, bytes, first_msg;
>     PopStatus status = POP_OK;
> 
>     g_return_val_if_fail(pop_host, POP_HOST_NOT_FOUND);
>     g_return_val_if_fail(pop_user, POP_AUTH_FAILED);
> 
> 
>     DM("POP3 - begin connection");
>     status = pop_connect(&s, pop_host, pop_port);
>     DM("POP3 Connected; hello");
> 
>    if(status == POP_OK)
> 	status = pop_auth(s, pop_user, pop_pass, use_apop);
> 
>     DM("POP3 authentication %s successful", (status ==POP_OK ? "" :
> "NOT"));
>     if(status == POP_OK)
> 	status = pop_get_stats(s, &first_msg, &msgs, &bytes,  
> 			       delete_on_server ? NULL : uid, last_uid);
>     DM("POP3 status after get stats: %d", status);
>     if(status == POP_OK)
> 	status = proccb(s, first_msg, msgs, bytes, delete_on_server, 
> 			data, prog_cb);
>     
>     if(status != POP_CONN_ERR) {
> 	DM("POP3 C: quit");
> 	/* exit gracefully */
> Bug---->write (s, "quit\r\n", 6);
> 	getLine (s, buffer, sizeof (buffer)); /* snarf the response */
> 	if(status == POP_OK)
> 	    strcpy(last_uid, uid);/* FIXME: overflow error on hideous
> reply? */
>     }
>     close (s);
> 
>     return status;
> }

This code is heinous. It should do the classic linux-kernel thing:
   status = pop_connect...
   if (status != POP_OK)
      goto conn_fail;
   status = pop_auth...
   if (status != POP_OK)
      goto cleanup_fail;

   ...

   close(s);
   return status;

cleanup_fail:
   write("quit...
   ...
   close(s);
   // fall through

conn_fail
    return status;

-------------------
That way you can be sure that you really do the right thing, otherwise
it is far too easy to make an error.

btw: the bug is unlikely to cause catastrophic failure, though closing
the wrong fd might do that.
	-gordo