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

[niuxu]

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!

[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.

Post by niuxu
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.
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;
}
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