:: Re: [Libbitcoin] overflow bug regre…
Top Page
Delete this message
Reply to this message
Author: Eric Voskuil
Date:  
To: 'Amir Taaki'
CC: libbitcoin
New-Topics: [Libbitcoin] dllexport of libbitcoin
Subject: Re: [Libbitcoin] overflow bug regression
Below is the list of warnings currently in libbitcoin, from the msvc compiler at warning level 3 (default) in a x64 build. I previously eliminated a bunch, but these that remain require a little more thought. Either the warning level I'm using is more stringent or size_t is being defined as 32 bit on other builds, but the warnings highlight legitimate potential issues.

49    warning C4267: 'argument' : conversion from 'size_t' to 'uint32_t', possible loss of data
75    warning C4267: '+=' : conversion from 'size_t' to 'uint32_t', possible loss of data
167    warning C4267: 'argument' : conversion from 'size_t' to 'int', possible loss of data
54    warning C4267: '=' : conversion from 'size_t' to 'uint32_t', possible loss of data (..\..\..\..\src\getx_responder.cpp)
284    warning C4267: 'argument' : conversion from 'size_t' to 'uint32_t', possible loss of data (..\..\..\..\src\network\channel.cpp)
54    warning C4267: '=' : conversion from 'size_t' to 'uint32_t', possible loss of data (..\..\..\..\src\network\channel.cpp)
112    warning C4267: 'return' : conversion from 'size_t' to 'int', possible loss of data
54    warning C4267: '=' : conversion from 'size_t' to 'uint32_t', possible loss of data (..\..\..\..\src\network\handshake.cpp)
510    warning C4267: 'argument' : conversion from 'size_t' to 'uint32_t', possible loss of data
610    warning C4267: 'argument' : conversion from 'size_t' to 'uint32_t', possible loss of data
2069    warning C4267: 'argument' : conversion from 'size_t' to 'uint32_t', possible loss of data
2082    warning C4267: 'argument' : conversion from 'size_t' to 'uint8_t', possible loss of data
2084    warning C4267: 'argument' : conversion from 'size_t' to 'uint16_t', possible loss of data
2086    warning C4267: 'argument' : conversion from 'size_t' to 'uint32_t', possible loss of data
2104    warning C4267: '=' : conversion from 'size_t' to 'uint8_t', possible loss of data
54    warning C4267: '=' : conversion from 'size_t' to 'uint32_t', possible loss of data (..\..\..\..\src\network\protocol.cpp)
54    warning C4267: '=' : conversion from 'size_t' to 'uint32_t', possible loss of data (..\..\..\..\src\poller.cpp)
97    warning C4267: 'argument' : conversion from 'size_t' to 'uint32_t', possible loss of data
54    warning C4267: '=' : conversion from 'size_t' to 'uint32_t', possible loss of data (..\..\..\..\src\session.cpp)
167    warning C4267: '=' : conversion from 'size_t' to 'uint32_t', possible loss of data
92    warning C4244: 'argument' : conversion from '__int64' to 'uint32_t', possible loss of data
105    warning C4267: 'argument' : conversion from 'size_t' to 'int', possible loss of data
138    warning C4267: 'initializing' : conversion from 'size_t' to 'unsigned int', possible loss of data
140    warning C4267: 'argument' : conversion from 'size_t' to 'int', possible loss of data
151    warning C4267: 'argument' : conversion from 'size_t' to 'long', possible loss of data
178    warning C4267: 'argument' : conversion from 'size_t' to 'int', possible loss of data
173    warning C4244: 'return' : conversion from 'unsigned __int64' to 'uint32_t', possible loss of data
258    warning C4267: 'argument' : conversion from 'size_t' to 'uint32_t', possible loss of data
861    warning C4267: 'initializing' : conversion from 'size_t' to 'uint32_t', possible loss of data
877    warning C4267: 'argument' : conversion from 'size_t' to 'uint32_t', possible loss of data


Using size_t isn't always an option or appropriate. I'm not saying there are actual bugs here, but allowing these to persist can mask real potential problems down the road.

e

-----Original Message-----
From: Amir Taaki [mailto:genjix@riseup.net]
Sent: Sunday, April 06, 2014 1:24 PM
To: Eric Voskuil
Cc: Amir Taaki; libbitcoin@???
Subject: Re: overflow bug regression

I'm compiling and exclusively using 64bit.
Generally C++ style casts should be used instead of C casts.
But otherwise I think apart from these few cases (that are now fixed), the
code is generally using size_t.

> Yes, my mistake - thanks. The change should have been to make step and
> start int (vs. size_t).
>
> There is quite a bit of implicit coercion between fixed length and
> architecture-based integer variables, in both the libbitcoin and zmq
> projects. I always compile for 32 and 64 bit and it seems evident that
> these problems would be caught early if developers regularly compiled
> for 64 bit architecture.
>
> I've made a number of corrections in areas that looks safe (despite this
> mistake) although others require deeper surgery to be done properly. I'm
> not comfortable doing that without a comprehensive test suite or at
> least an ability to manually investigate each code path (which I'm not
> up to yet).
>
> If data types are being chosen for the right reasons then explicit
> casting should be used when conversion is necessary and intended. And
> when an cast occurs it's a sign that an upstream guard must be in place
> to handle the potential error.
>
> e
>
> On 04/06/2014 08:56 AM, Amir Taaki wrote:
>> hey eric,
>>
>> I had to change this as it was causing obelisk to throw std::bad_alloc.
>>
>> https://github.com/spesmilo/libbitcoin/commit/1f8cdf58c3a42ccccfa33516d528b000aee0a53e
>>
>
>