Re: ssmtp on the Hurd

[Guillem Jover]

Hi,

On Mon, Jan 24, 2005 at 02:18:31PM +0100, Christopher Bodenstein wrote:
> Here is a patch to build ssmtp (a simple SMTP forwarder) on the Hurd. 
> 
> Any comments and suggestions would bemost welcome. :) 

> FWIW, I'm using it without any problems since Saturday (but it's a
> rather light usage though).

Ok some comments:

Just remove the old code (do not comment it), the changes can be seen
anyway in the patch.

> --- ssmtp.c   2005-01-22 20:09:02.000000000 +0100
> +++ ssmtp.c   2005-01-22 20:08:46.000000000 +0100

> @@ -59,7 +61,8 @@
>  char *auth_method = (char)NULL;              /* Mechanism for SMTP 
> authentication */
>  char *mail_domain = (char)NULL;
>  char *from = (char)NULL;             /* Use this as the From: address */
> -char hostname[MAXHOSTNAMELEN] = "localhost";
> +/* char hostname[MAXHOSTNAMELEN] = "localhost";  */
> +char *hostname = "localhost";
>  char *mailhost = "mailhub";
>  char *minus_f = (char)NULL;
>  char *minus_F = (char)NULL;

See the comments below about hostname.

> @@ -140,7 +143,8 @@
>  */
>  void dead_letter(void)
>  {
> -     char path[(MAXPATHLEN + 1)], buf[(BUF_SZ + 1)];
> +     /* char path[(MAXPATHLEN + 1)], */
> +     char buf[(BUF_SZ + 1)];
>       struct passwd *pw;
>       uid_t uid;
>       FILE *fp;
> @@ -148,6 +152,8 @@
>       uid = getuid();
>       pw = getpwuid(uid);
>  
> +     char path[(sizeof(pw->pw_dir) + 1)];
> +
>       if(isatty(fileno(stdin))) {
>               if(log_level > 0) {
>                       log_event(LOG_ERR,

This change is wrong in various ways. First you have added a variable
declaration after some code, that's C99 and will not build on older
compilers. Also you are using sizeof on a "char *", that will give you
the pointer size on compile time, not the size of what it's pointing
to, so on 32 bit arches for example it will return 4. Also don't just
subst sizeof with strlen, setting the variable's size on runtime is a
GNU extension or C99. You need to declare the variable, then allocate
the needed memory (see malloc and strlen) and when leaving the
function or when it exits or dies make sure to free the memory.

> @@ -218,19 +224,23 @@
>  */
>  char *basename(char *str)
>  {
> -     char buf[MAXPATHLEN +1], *p;
> +     /* char buf[MAXPATHLEN +1], *p; */
> +     char buf[sizeof(str) +1], *p;
>  
>       if((p = strrchr(str, '/'))) {
> -             if(strncpy(buf, ++p, MAXPATHLEN) == (char *)NULL) {
> +             /* if(strncpy(buf, ++p, MAXPATHLEN) == (char *)NULL) { */
> +             if(strncpy(buf, ++p, sizeof(str)) == (char *)NULL) {
>                       die("basename() -- strncpy() failed");
>               }
>       }
>       else {
> -             if(strncpy(buf, str, MAXPATHLEN) == (char *)NULL) {
> +             /* if(strncpy(buf, str, MAXPATHLEN) == (char *)NULL) { */
> +             if(strncpy(buf, str, sizeof(str)) == (char *)NULL) {
>                       die("basename() -- strncpy() failed");
>               }
>       }
> -     buf[MAXPATHLEN] = (char)NULL;
> +     /* buf[MAXPATHLEN] = (char)NULL; */
> +     buf[sizeof(str)] = (char)NULL;
>  
>       return(strdup(buf));
>  }

That's wrong as well. Refer to the above comment about sizeof vs.
strlen and dynamic memory allocation.

> @@ -830,8 +840,9 @@
>                               }
>                       }
>                       else if(strcasecmp(p, "HostName") == 0) {
> -                             if(strncpy(hostname, q, MAXHOSTNAMELEN) == 
> NULL) {
> -                                     die("parse_config() -- strncpy() 
> failed");
> +                             /* if(strncpy(hostname, q, MAXHOSTNAMELEN) == 
> NULL) { */
> +                             if(strcpy(hostname, q) == NULL) { 
> +                                     die("parse_config() -- strcpy() 
> failed");
>                               }
>  
>                               if(log_level > 0) {

This change is wrong. Your are assigning to a variable that points
to a constant, so you need to dynamically allocate the needed space
when overwritting... but then you have to free the memory if you
allocated it, but only *if* it was allocated, not in case it's using
the "localhost" constant.

> @@ -1885,9 +1896,16 @@
>       /* Set the globals */
>       prog = basename(argv[0]);
>  
> +     hostname = xgethostname();
> +     if(!hostname) {
> +             perror("xgethostname");
> +             die("Cannot get the name of this machine");
> +     }
> +     /* Don't need this anymore 
>       if(gethostname(hostname, MAXHOSTNAMELEN) == -1) {
>               die("Cannot get the name of this machine");
>       }
> +     */
>       new_argv = parse_options(argc, argv);
>  
>       exit(ssmtp(new_argv));

Here as well, you have to free what xgethostname returned
Another problem is that it's not freeing mailhost either (that's
an existing problem in the source, not introduced by you).

Thanks for taking care of this. :>

regards,
guillem