Re: [Bug-wget] Recommendations for adding log statements after checking setsockopt()

[Darshit Shah]

Hi,

Thanks for the analysis! However, the code that you have identified comes from
gnulib, which is essentially a statically linked library. As a result code in
there is out of bounds for us to change.

Also, if you see closely, the function you've pointed to, is the
"rpl_setsockopt", that is, it is a replacement function for systems where
setsockopt doesn't behave in a sane manner. Hence, all the invokations to
setsockopt are indeed checked and logged.

* niuxu <address@hidden> [181015 11:56]:
> Our team works on enhance logging practices by learning from historical log 
> revisions in evolution.
> We find that 2 patches have added validation code about the return value of 
> setsockopt() along with logging statements. 
> 
> 
> So we suggest that the return value of setsockopt() should be checked and 
> logged if the check pass.
> 
> And, we find 1 missed spot in line 35 of wget-1.19.2/lib/setsockopt.c:
> int
> rpl_setsockopt (int fd, int level, int optname, const void *optval, socklen_t 
> optlen)
> {
>   ...
>   if (level == SOL_SOCKET
>       && (optname == SO_RCVTIMEO || optname == SO_SNDTIMEO))
>      {
>         const struct timeval *tv = optval;
>         int milliseconds = tv->tv_sec * 1000 + tv->tv_usec / 1000;
>         optval = &milliseconds;
>         r = setsockopt (sock, level, optname, optval, sizeof (int));
>      }
>   else
>      {
>         r = setsockopt (sock, level, optname, optval, optlen);
>      }
>   if (r < 0)
>      set_winsock_errno ();
> 
>   return r;
> }
> 
> And the 2 patches that support us are:
> 1) In line 334 of File: wget-1.18/src/connect.c
>      if (opt.limit_rate && opt.limit_rate < 8192)
>      {
>        int bufsize = opt.limit_rate;
>        if (bufsize < 512)
>          bufsize = 512;          /* avoid pathologically small values */
>      #ifdef SO_RCVBUF
> -      setsockopt (sock, SOL_SOCKET, SO_RCVBUF,
> -                  (void *)&bufsize, (socklen_t)sizeof (bufsize));
> +      if (setsockopt (sock, SOL_SOCKET, SO_RCVBUF,
> +                  (void *) &bufsize, (socklen_t) sizeof (bufsize)))
> +        logprintf (LOG_NOTQUIET, _("setsockopt SO_RCVBUF failed: %s\n"),
> +                   strerror (errno));
>      #endif
> 
> 2) In line 474 of File:  wget-1.18/src/connect.c
>    sock = socket (bind_address->family, SOCK_STREAM, 0);
>    if (sock < 0)
>      return -1;
>  
>  #ifdef SO_REUSEADDR
> -  setsockopt (sock, SOL_SOCKET, SO_REUSEADDR, setopt_ptr, setopt_size);
> +  if (setsockopt (sock, SOL_SOCKET, SO_REUSEADDR, setopt_ptr, setopt_size))
> +    logprintf (LOG_NOTQUIET, _("setsockopt SO_REUSEADDR failed: %s\n"),
> +               strerror (errno));
>  #endif
> 
> Thanks for your reading and we are looking forward to your reply about the 
> correctness of our suggestion.
> May you a good day! ^^
> 
> Best Regards,
> Xu

-- 
Thanking You,
Darshit Shah
PGP Fingerprint: 7845 120B 07CB D8D6 ECE5 FF2B 2A17 43ED A91A 35B6

signature.asc
Description: PGP signature