Opened 12 years ago

Closed 5 years ago

#4636 closed Bugs (fixed)

Run-Time Check Failure #1 in feed_args.hpp using Visual Studio 2010

Reported by: Rüdiger Brünner <rbruenner@…> Owned by: James E. King, III
Milestone: Boost 1.66.0 Component: format
Version: Boost 1.43.0 Severity: Problem
Keywords: RTC VS2010 MSVC10 Cc:

Description

Hello,

the statement

size_type res_size = (std::min)(

static_cast<size_type>(specs.truncate_ - !!prefix_space), buf.pcount() );

on line 166 ff. causes an error message in Debug mode in VS 2010 when run-time checks are enabled:

"Run-Time Check Failure #1 - A cast to a smaller data type has caused a loss of data. If this was intentional, you should mask the source of the cast with the appropriate bitmask. For example:

char c = (i & 0xFF);

Changing the code in this way will not affect the quality of the resulting optimized code."

The value of specs.truncate_ (64 bits) is LLONG_MAX that is going to be cast down to a size_t (32 bits). This causes that warning. Would it be feasible to initalise the truncate_ member to INT_MAX on 32 bit systems to avoid this issue?

This message is *very* annoying since it pops up very frequently.

Best regards Rüdiger

Change History (22)

comment:1 by Steven Watanabe, 12 years ago

FWIW, I advise against enabling run-time checks, since the result is non-conformant. The behavior of this cast is well defined according to the standard.

comment:2 by Rüdiger Brünner <rbruenner@…>, 12 years ago

Yes, I understand that these run-time checks produce a lot of noise. They are good for finding obscure bugs but I no longer use them permanently.

Best regards

comment:3 by kurt@…, 12 years ago

We just ran into this as well. That type check is useful for debugging, and it would be great if Boost played nice with it.

comment:4 by Gordon.Geddes@…, 11 years ago

We've also just come across this problem. By default we run our code in debug with all runtime checks as they can be useful to highlight potential problems but with boost not working with them we're having to switch them off in affected projects. It would be nice if boost did work with these runtime checks.

comment:5 by anonymous, 11 years ago

I just ran into this very annoying problem as well.

The cast may be well defined by the standard, but there are quite a few situations where you want to detect overflowing a 32-bit variable with a 64-bit value. So those checks do make sense to me.

I second that it would be nice if boost worked with these checks.

-Matthias

comment:6 by dxc@…, 11 years ago

it's still present in boost 1.48.0. we run our regressions on both debug and release builds, and for debug we turn on all the runtime checks. it is well and fine that the standard defines the way the cast is supposed to work, but it is also true that sometimes this runtime check catches a programmer error. we have caught a few corner-cases using this check that otherwise would have been lurking until the customer found it, no doubt at the worst possible time.

sure, I can add the masking trick myself, but 1) not having to mess around in the code is one reason to use high quality libraries; and 2) if *I* patch it now I have to remember to patch every time, and if the code changes this may be non-trivial.

can this really not be fixed? it has been there since at least 1.38 according to the boost users mailing list.

dxc

comment:7 by anonymous, 10 years ago

Still there in 1.49. I've told VS2010 to not do any exception checks in Debug mode and it still pops up a dialog box. Very annoying. Works just fine in Release mode, tho'.

comment:8 by anonymous, 9 years ago

Still an issue in 1.53. We added the masking trick in when we were using 1.48, but forgot we did when we upgraded and were confused when our unit test failed. Wasted time. Can we please fix this?

comment:9 by emmanuel.andre@…, 9 years ago

Same issue here... We had to deactivate runtime checking of overflows for the whole impacted component.

When can we expect a fix ? Do you want that we propose/work on a solution ?

Best regards

comment:10 by timblechmann, 9 years ago

very annoying issue. even if the behaviour of the cast may be well defined according to the standard, it would not hurt to implement it explicitly to avoid msvc firing on a potential false positive.

comment:11 by timblechmann, 9 years ago

i'd suggest the following patch:

--- a/boost/format/feed_args.hpp
+++ b/boost/format/feed_args.hpp
@@ -21,6 +21,7 @@
 #include <boost/format/format_class.hpp>
 #include <boost/format/group.hpp>
 #include <boost/format/detail/msvc_disambiguater.hpp>
+#include <boost/integer_traits.hpp>
 
 namespace boost {
 namespace io {
@@ -171,8 +172,8 @@ namespace detail {
                    (res_beg[0] !=oss.widen('+') && res_beg[0] !=oss.widen('-')  ))
                     prefix_space = oss.widen(' ');
             size_type res_size = (std::min)(
-                static_cast<size_type>(specs.truncate_ - !!prefix_space), 
-                buf.pcount() );
+                static_cast<size_type>(specs.truncate_ & integer_traits<size_type>::const_max - !!prefix_space), 
+                static_cast<size_type>(buf.pcount()) );
             mk_str(res, res_beg, res_size, w, oss.fill(), fl, 
                    prefix_space, (specs.pad_scheme_ & format_item_t::centered) !=0 );
         }

comment:12 by anonymous, 9 years ago

Hi,

Whether this issue is fixed in latest version of boost library 1.55?.When we apply above mentioned patch is that any thing compromised?. I am facing this issue at run time only. Does the above patch leads to any data loss? Because we are making application level changes to support Unicode format(to support all languages).

i.e wcout<<boost::wformat("My name is %s") %username

comment:13 by adam.f.badura@…, 7 years ago

The problem still exists in Boost 1.58.

comment:13 by adam.f.badura@…, 7 years ago

The problem still exists in Boost 1.58.

comment:14 by anonymous, 6 years ago

The problem still exists in Boost 1.63

comment:14 by anonymous, 6 years ago

The problem still exists in Boost 1.63

comment:15 by James E. King, III, 5 years ago

Owner: changed from Samuel Krempp to James E. King, III

I am looking into this.

Event without /RTCsuc I am able to see the same thing using a numeric_cast as follows:

            size_type res_size = (std::min)(
                numeric_cast<size_type>(specs.truncate_) - !!prefix_space, 
                buf.pcount() );

Which results in the error:

class boost::numeric::positive_overflow: bad numeric conversion: positive overflow

Inspecting it in the debugger I see:

-		specs	{argN_=0 res_="" appendix_="00" ...}	const boost::io::detail::format_item<char,std::char_traits<char>,std::allocator<char> > &
		argN_	0	int
+		res_	""	std::basic_string<char,std::char_traits<char>,std::allocator<char> >
+		appendix_	"00"	std::basic_string<char,std::char_traits<char>,std::allocator<char> >
+		fmtstate_	{width_=0 precision_=6 fill_=32 ' ' ...}	boost::io::detail::stream_format_state<char,std::char_traits<char> >
		truncate_	9223372036854775807	__int64
		pad_scheme_	0	unsigned int

The offending test is in format_test1.cpp:

  if(str( format("%%##%%##%%1 %1%00") % "Escaped OK" ) != "%##%##%1 Escaped OK00")

At first glance I would say that the value of truncate_ is wrong here, and figuring that out will end up being the root cause.

comment:16 by James E. King, III, 5 years ago

format_item::truncate_ uses a std::streamsize which is an unsigned long long type, and it is initialized to a max() value for that type if the format specification doesn't have explicit truncation defined. The code that can make a string (mk_str) has a maximum size of std::string::size_type which is an unsigned long. This impedance mismatch is really the core of the issue here. The code is relying on a clamping mechanism that ensures a cast of the maximum stream size is a useful value when truncated to 32-bits. Therefore it looks like the code in the patch is partially correct; there are other instances that need to be addressed so I will work on this.

comment:18 by James E. King, III, 5 years ago

Keywords: RTC MSVC10 added; format Run-Time Check Failure removed
Milestone: To Be DeterminedBoost 1.66.0
Status: newassigned

comment:19 by James E. King, III, 5 years ago

Merged into develop, will resolve this when develop merges into master.

comment:20 by James E. King, III, 5 years ago

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.