Re: Bug in POP3 code

[Emmanuel <e allaud wanadoo fr>]

On 2001.10.06 01:53 Gordon Oliver wrote:
> 
> 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

I had not seen your post before sending my patch. If it is to be included
because if solves anythin ;-) and if necessary I'll rewrite the "kernel"
way.
Thanks
Bye
Manu