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 majord...@freehaven.net with unsubscribe libevent-users in the body.
