Re: [Libevent-users] [patch] integer recommendations

[Mark Ellzey <mthomas@xxxxxxxxxx>]

On Mon, Apr 23, 2012 at 09:04:59AM -0400, Mansour Moufid wrote:
> Hi,
> 
> Here's a patch to adhere to a few of the integer-related recommendations in the CERT C Secure Coding Standard. I tried not to break anything but you may want to double-check.
> 
> Mansour
> 

It seems as if the tool being used to generate this report is very generic
scanner. One without the ability to follow logical flows or understand
function calls outside the base source.

seed_urandom, only has 3 filenames, and a buflen of 32 bytes (cleared with each loop). 
The loop will only happen 3 times, and the filenames are not > 32 bytes.

base_with_config only has 6 entries, this will never overflow.
get_supported_methods only has 6 entries, this will never overflow.
socket_error_to_string only has 50 or so entries, this will never overflow.

evhttp_decode_uri_internal, well, the tool missed bugs which could be considered
worse. These other bugs will be fixed in the next release.

evhttp_response_phrase_internal integer logic is already checked for. Also in
the case of this patch, an upcast doesn't fix the issue, just hides it from the
tool you are using it.

evutil_parse_sockaddr_port, isn't this already checked by if (port <= 0 || port > 65535)? 
Also strtol returns long int which in some cases would also overflow. I think
this just masks the error from your tool. 

evhttp_get_request_connection same as above, but you're casting to uint16_t,
which could make the problem worse. evhttp_connection_base_new()'s port argument
expects unsigned short. This logic doesn't work.

evhttp_parse_response_line, response_code expects int, strtol returns long int.

devpoll_dispatch, ioctl returns an int, accessing events[i].revents could cause
problems with unsigned int.

poll_dispatch, poll() returns int, revents is short, this is wrong, and once again 
this patch just hides warnings from your tool.

epoll_dispatch, epoll_wait() returns an int, accessing events[i].events could
now be problimatic. 

evutil_inet_pton, return is checked right afterwards, but.. This might be an apt
fix.

evutil_make_socket_closeonexec, fcntl returns int, which you are checking for
unsigned. This just makes your tool happy.

evutil_make_socket_nonblocking, same issue as above.

poll_dispatch, poll() returns int, revents is short, this is wrong, and once
again this patch just hides warnings from your tool.

send_document_cb, since I am guessing the point of the patch is to make sure
dirlen+3 does not wrap, just putting them into two variables doesn't actually
fix the problem. 

check_dummy_mem_ok same issue as above.

TL;DNR:

These fixes have the potential to break more things they than actually fix. Some of them 
not even fixing things at all, but just masking them..

Don't get me wrong, we should definitely look at coming up with
standards of integer uses, but in some cases are unavoidable (such as
returns from functions outside of libevent). But this is a topic for the
future, and not as easy as the patches here..
***********************************************************************
To unsubscribe, send an e-mail to majordomo@xxxxxxxxxxxxx with
unsubscribe libevent-users    in the body.