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

[Mansour Moufid <mansourmoufid@xxxxxxxxx>]

On 2012-04-23, at 12:33 PM, Mark Ellzey wrote:

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

Yes, perhaps here is not the place.

You're right that none of these address specific issues, and I should have been more sensitive to this being stable code.

I did study each case specifically (obviously not enough) but the sum may have come off as rash. I appreciate the feedback. Thanks for taking the time.

Mansour

***********************************************************************
To unsubscribe, send an e-mail to majordomo@xxxxxxxxxxxxx with
unsubscribe libevent-users    in the body.