Fix rlp tool failing to parse long hex string #5548

twinstar26 · 2019-04-05T16:51:02Z

jwasinger · 2019-04-05T16:59:25Z

rlp/main.cpp

+    {
+        //filesyatem_error contains lot of error flags but I couldn't find file name too long
+        //the closest I got was boost::system::errc -> enum errc_t ->  filename_too_long = ENAMETOOLONG
+        if(_e.what() == "File name too long") //don't know if this will work. Let's say temporary workaround.


Because what() returns a char * you can't compare directly to a string literal here. You need to wrap the result of what in a string object before doing the comparison.

Thanks for pointing out. My bad, sorry.
But the problem still remains unaddressed, if what I am doing (Directly comparing strings) will solve this issue.
I will correct my error soon.

codecov-io · 2019-04-06T03:03:06Z

Codecov Report

Merging #5548 into release/1.6 will decrease coverage by 0.03%.
The diff coverage is 65.38%.

@@               Coverage Diff               @@
##           release/1.6    #5548      +/-   ##
===============================================
- Coverage        61.98%   61.94%   -0.04%     
===============================================
  Files              344      344              
  Lines            28778    28782       +4     
  Branches          3269     3271       +2     
===============================================
- Hits             17838    17830       -8     
- Misses            9769     9776       +7     
- Partials          1171     1176       +5

jwasinger · 2019-04-06T04:01:51Z

rlp/main.cpp

+        string error_e = _e.what();
+        string long_file_name = "boost::filesystem::status: File name too long: \"";
+        long_file_name += inputFile + "\"";
+        if(error_e == long_file_name)


You can probably assume that the input should be treated as bytecode if you get any error from is_regular_file. Probably don't need to check the error message or type.

And checking for error types by comparing against hard coded strings is bad form because it will break if the error message is changed upstream in Boost.

Yeah I totally agree with you regarding the hard coded strings and that change in upstream boost will break code.
But,
I thought that in here

aleth/rlp/main.cpp

Lines 259 to 265 in b09e9ad

if (inputFile == "--")

for (int i = cin.get(); i != -1; i = cin.get())

in.push_back((byte)i);

else if (boost::filesystem::is_regular_file(inputFile))

in = contents(inputFile);

else

in = asBytes(inputFile);

contents() internally call

aleth/libdevcore/CommonIO.cpp

Lines 86 to 105 in ad7204c

template <typename _T>

inline _T contentsGeneric(boost::filesystem::path const& _file)

{

_T ret;

size_t const c_elementSize = sizeof(typename _T::value_type);

boost::filesystem::ifstream is(_file, std::ifstream::binary);

if (!is)

return ret;

// get length of file:

is.seekg(0, is.end);

streamoff length = is.tellg();

if (length == 0)

return ret; // do not read empty file (MSVC does not like it)

is.seekg(0, is.beg);

ret.resize((length + c_elementSize - 1) / c_elementSize);

is.read(const_cast<char*>(reinterpret_cast<char const*>(ret.data())), length);

return ret;

}

contentsGeneric() and here it's performing boost::filesystem operations so boost::filesystem::filesystem_error may occur and then catching every error and handling them as filename_too_long will create another bug.

Please correct me if I am wrong.
Also I have some ideas, but I need some time to select the best out of them.

twinstar26 · 2019-04-07T15:12:56Z

Hi,
I guess this latest commit will solve this issue.
Also if there are any exceptions to be handled please let me know.

gumb0

I see a bug: in case is_regular_file returns false, in stays empty and we don't parse anything.

Did you try to run your fix?

gumb0 · 2019-04-08T10:30:13Z

rlp/main.cpp

+        else if (boost::filesystem::is_regular_file(inputFile))
+            in = contents(inputFile);
+    }
+    catch(const boost::filesystem::filesystem_error& _e)


Suggested change

catch(const boost::filesystem::filesystem_error& _e)

catch (const boost::filesystem::filesystem_error& e)

See https://github.com/ethereum/aleth/blob/master/CODING_STYLE.md

gumb0 · 2019-04-08T10:35:13Z

rlp/main.cpp

+    }
+    catch(const boost::filesystem::filesystem_error& _e)
+    {
+        if(_e.code() == boost::system::errc::errc_t::filename_too_long)


It's ok to handle all file system errors the same way (errors from contents(), too), not only filename_too_long

gumb0 · 2019-04-08T10:37:15Z

rlp/main.cpp

-    else
-        in = asBytes(inputFile);
-
+    try


Don't make try-catch block wider than it needs to be - if you intend to handle errors from is_regular_file only (maybe together with contents()) - wrap only those functions in try-catch

twinstar26 · 2019-04-08T16:24:02Z

@gumb0
So keeping in mind all the past comments,
try
{
if (inputFile == "--")
for (int i = cin.get(); i != -1; i = cin.get())
in.push_back((byte)i);
else if (boost::filesystem::is_regular_file(inputFile))
in = contents(inputFile);
else
{
in = asBytes(inputFile);
}
}
catch (const boost::filesystem::filesystem_error& e)
{
in = asBytes(inputFile);
}

is my modified code.
But I am not creating a pull request as during my tests,
ubuntu18@ubuntu18-Inspiron-3268:~/opensource/aleth/build/rlp$ ./rlp f9037b04f8a8a00064e8a4f925e944185338c03a1b913c3bcc7e762f5ba5bde22fc17df59215c5f885b84017163d503ea69bca3d349654b21080e2431cf1102c8907841b43efa96e33b76012a3f8a1ec1851945d077a7e6b13d577f85892150a8fbd7579f539a06c53ee508082696476348269700d4abd93887365633235366b31036bffe5e6caa66422eccb705c6f1d18ec272b7e21d5648d2ca213be1888b3117083746370765f83756470765ff902cdf8578434e8f39882765f82765fb8406332792c4a00e3e4ee0926ed89e0d27ef985424d97b6a45bf0f23e51f0dcb5e66b875777506458aea7af6f9e4ffb69f43f3778ee73c81ed9d34c51c4b16b0b0f845ca24b9d845ca24b9df857843410bcb982765f82765fb840a979fb575495b8d6db44f750317d0f4622bf4c2aa3365d6af7c284339968eef29b69ad0dce72a4d8db5ebb4968de0e3bec910127f134779fbcb0cb6d3331163c845ca24b9c845ca24b9cf857840d4b9a8a82765f82765fb840158f8aab45f6d19c6cbf4a089c2670541a8da11978a2f90dbf6a502a4a3bab80d288afdbeb7ec0ef6d92de563767f3b1ea9e8e334ca711e9f8e2df5a0385e8e6845ca24b9c845ca24b9cf8578434b0644d82765f82765fb840865a63255b3bb68023b6bffd5095118fcc13e79dcf014fe4e47e065c350c7cc72af2e53eff895f11ba1bbb6a2b33271c1116ee870f266618eadfc2e78aa7349c845ca24b9c845ca24b9ff8578434b0070a82765f82765fb84015ac307a470b411745a6f10544ed54c0a14ad640b21f04f523e736e732bf709d4e28c2f06526ecabc03eed226b6d9bee8e433883cd20ab6cbd114bab77a8775d845ca24bb5845ca24b9cf857840d5dd35482765f82765fb840aa36fdf33dd030378a0168efe6ed7d5cc587fafa3cdd375854fe735a2e11ea3650ba29644e2db48368c46e1f60e716300ba49396cd63778bf8a818c09bded46f845ca24bb5845ca24b9cf85784344a397b82765f82765fb840669f45b66acf3b804c26ce13cfdd1f7e3d0ff4ed85060841b9af3af6dbfbacd05181e1c9363161446a307f3ca24e707856a01e4bf1eed5e1aefc14011a5c1c1c845ca24bb5845ca24b9cf85c8434b0644d82765f82765fb840865a63255b3bb68023b6bffd5095118fcc13e79dcf014fe4e47e065c350c7cc72af2e53eff895f11ba1bbb6a2b33271c1116ee870f266618eadfc2e78aa7349c01845ca24bb4845ca24bba04108080
[ "0x04", [ "0x0064e8a4f925e944185338c03a1b913c3bcc7e762f5ba5bde22fc17df59215c5", [ "0x17163d503ea69bca3d349654b21080e2431cf1102c8907841b43efa96e33b76012a3f8a1ec1851945d077a7e6b13d577f85892150a8fbd7579f539a06c53ee50", "0x", "0x6964", "0x76", "0x34", "0x6970", "0x0d", "0x4a"Error: Invalid format; bad RLP.
After a lot of debugging i found that

aleth/rlp/main.cpp

Lines 412 to 418 in a611afe

    
           case Mode::Render: 
        
           { 
        
               RLPStreamer s(cout, prefs); 
        
               s.output(rlp); 
        
               cout << endl; 
        
               break; 
        
           }

is getting executed and as a result its falling into this catch block

aleth/rlp/main.cpp

Lines 482 to 486 in a611afe

    
           catch (...) 
        
           { 
        
               cerr << "Error: Invalid format; bad RLP." << endl; 
        
               exit(AlethErrors::BadRlp); 
        
           }

Declaration can be found over here

aleth/rlp/main.cpp

Lines 74 to 108 in a611afe

    
           RLPStreamer(ostream& _out, Prefs _p): m_out(_out), m_prefs(_p) 
        
           { 
        
               if (_p.hexInts) 
        
                   _out << std::hex << std::showbase << std::nouppercase; 
        
           } 
        
           void output(RLP const& _d, unsigned _level = 0) 
        
           { 
        
               if (_d.isNull()) 
        
                   m_out << "null"; 
        
               else if (_d.isInt() && !m_prefs.stringInts) 
        
                   m_out << _d.toInt<bigint>(RLP::LaissezFaire); 
        
               else if (_d.isData() || (_d.isInt() && m_prefs.stringInts)) 
        
                   if (m_prefs.forceString || (!m_prefs.forceHex && isAscii(_d.toString()))) 
        
                       m_out << escaped(_d.toString(), m_prefs.escapeAll); 
        
                   else 
        
                       m_out << "\"" << toHexPrefixed(_d.toBytes()) << "\""; 
        
               else if (_d.isList()) 
        
               { 
        
                   m_out << "["; 
        
                   string newline = "\n"; 
        
                   for (unsigned i = 0; i < _level + 1; ++i) 
        
                       newline += m_prefs.indent; 
        
                   int j = 0; 
        
                   for (auto i: _d) 
        
                   { 
        
                       m_out << (j++ ? 
        
                           (m_prefs.indent.empty() ? ", " : ("," + newline)) : 
        
                           (m_prefs.indent.empty() ? " " : newline)); 
        
                       output(i, _level + 1); 
        
                   } 
        
                   newline = newline.substr(0, newline.size() - m_prefs.indent.size()); 
        
                   m_out << (m_prefs.indent.empty() ? (j ? " ]" : "]") : (j ? newline + "]" : "]")); 
        
               } 
        
           }

I am guessing this is not a normal situation...
Can you please provide me with more reference/insights regarding this new issue.

gumb0 · 2019-04-08T16:49:05Z

@twinstar26 That was incorrect RLP I gave in the issue description, so this output is actually expected, sorry for this confusion.

Try this instead:

rlp/rlp  f884b8407098ad865b00a582051940cb9cf36836572411a47278783077011599ed5cd16b76f2635f4e234738f30813a89eb9137e3e3df5266e3a1f11df72ecf1145ccb9c01826964827634826970847f00000189736563703235366b31a103ca634cae0d49acb401d8a4c6b6fe8c55b70d115bf400769cc1400f3258cd31388375647082765f

twinstar26 · 2019-04-08T16:55:24Z

@gumb0
So i tried with the new RLP and this is my new output
ubuntu18@ubuntu18-Inspiron-3268:~/opensource/aleth/build/rlp$ ./rlp f884b8407098ad865b00a582051940cb9cf36836572411a47278783077011599ed5cd16b76f2635f4e234738f30813a89eb9137e3e3df5266e3a1f11df72ecf1145ccb9c01826964827634826970847f00000189736563703235366b31a103ca634cae0d49acb401d8a4c6b6fe8c55b70d115bf400769cc1400f3258cd31388375647082765f [ "0x7098ad865b00a582051940cb9cf36836572411a47278783077011599ed5cd16b76f2635f4e234738f30813a89eb9137e3e3df5266e3a1f11df72ecf1145ccb9c", "0x01", "0x6964", "0x7634", "0x6970", "0x7f000001", "0x736563703235366b31", "0x03ca634cae0d49acb401d8a4c6b6fe8c55b70d115bf400769cc1400f3258cd3138", "0x756470", "0x765f" ]
This time it did not enter catch block!!

twinstar26 · 2019-04-09T04:10:18Z

For this pull request,
Building failed on visual studio for commit "b695dee" due to " 'e': unreferenced local variable " (line 270)

Then for commit "1b2f222" Test Case "neighbours" failed (/Users/distiller/project/test/unittests/libp2p/net.cpp:1253: last checkpoint: "neighbours" test entry) on xcode

I failed to understand the connection between the two.

gumb0 · 2019-04-09T11:31:46Z

@twinstar26 there's no connection between these failures. Please ignore the second one (neighbours), this test is not stable lately (#5544 tracks this)

gumb0

@twinstar26 Please squash the commits (but pull my last commit first)

gumb0 · 2019-04-09T14:22:05Z

Rebased and added CHANGELOG item

gumb0 · 2019-04-09T15:24:10Z

@twinstar26 please remove the last merge commit, we don't usually merge from master into implementation branches, instead we rebase branches, this helps with keeping the commit log more linear.

This will be merged into master soon anyway.

twinstar26 · 2019-04-09T15:25:55Z

Yeah sorry I was updating my fork for solving some other issue.

twinstar26 · 2019-04-09T15:46:40Z

@gumb0 I reverted back. This was so embarrassing! 😅

gumb0 · 2019-04-09T17:13:42Z

Changed base to release/1.6 to include it in release.

jwasinger reviewed Apr 5, 2019

View reviewed changes

jwasinger reviewed Apr 6, 2019

View reviewed changes

gumb0 self-requested a review April 8, 2019 09:00

gumb0 suggested changes Apr 8, 2019

View reviewed changes

gumb0 changed the title ~~[WIP] rlp tool fails to parse long hex string #5543 exception handling~~ Fix rlp tool failing to parse long hex string Apr 9, 2019

gumb0 suggested changes Apr 9, 2019

View reviewed changes

gumb0 previously approved these changes Apr 9, 2019

View reviewed changes

gumb0 changed the base branch from master to release/1.6 April 9, 2019 17:04

Fix rlp tool failing to parse long hex string

9e37aba

gumb0 dismissed their stale review via 21a6804 April 9, 2019 17:11

Add changelog item

071dbfe

gumb0 approved these changes Apr 9, 2019

View reviewed changes

chfast merged commit 81827d2 into ethereum:release/1.6 Apr 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rlp tool failing to parse long hex string #5548

Fix rlp tool failing to parse long hex string #5548

twinstar26 commented Apr 5, 2019 •

edited

Loading

jwasinger Apr 5, 2019

twinstar26 Apr 5, 2019

codecov-io commented Apr 6, 2019 •

edited

Loading

jwasinger Apr 6, 2019

jwasinger Apr 6, 2019

twinstar26 Apr 6, 2019

twinstar26 commented Apr 7, 2019

gumb0 left a comment

gumb0 Apr 8, 2019

gumb0 Apr 8, 2019 •

edited

Loading

gumb0 Apr 8, 2019

twinstar26 commented Apr 8, 2019

gumb0 commented Apr 8, 2019

twinstar26 commented Apr 8, 2019

twinstar26 commented Apr 9, 2019 •

edited

Loading

gumb0 commented Apr 9, 2019

gumb0 left a comment

gumb0 commented Apr 9, 2019

gumb0 commented Apr 9, 2019

twinstar26 commented Apr 9, 2019

twinstar26 commented Apr 9, 2019

gumb0 commented Apr 9, 2019

	if (inputFile == "--")
	for (int i = cin.get(); i != -1; i = cin.get())
	in.push_back((byte)i);
	else if (boost::filesystem::is_regular_file(inputFile))
	in = contents(inputFile);
	else
	in = asBytes(inputFile);

	template <typename _T>
	inline _T contentsGeneric(boost::filesystem::path const& _file)
	{
	_T ret;
	size_t const c_elementSize = sizeof(typename _T::value_type);
	boost::filesystem::ifstream is(_file, std::ifstream::binary);
	if (!is)
	return ret;

	// get length of file:
	is.seekg(0, is.end);
	streamoff length = is.tellg();
	if (length == 0)
	return ret; // do not read empty file (MSVC does not like it)
	is.seekg(0, is.beg);

	ret.resize((length + c_elementSize - 1) / c_elementSize);
	is.read(const_cast<char>(reinterpret_cast<char const>(ret.data())), length);
	return ret;
	}

	catch(const boost::filesystem::filesystem_error& _e)
	catch (const boost::filesystem::filesystem_error& e)

Fix rlp tool failing to parse long hex string #5548

Fix rlp tool failing to parse long hex string #5548

Conversation

twinstar26 commented Apr 5, 2019 • edited Loading

jwasinger Apr 5, 2019

Choose a reason for hiding this comment

twinstar26 Apr 5, 2019

Choose a reason for hiding this comment

codecov-io commented Apr 6, 2019 • edited Loading

Codecov Report

jwasinger Apr 6, 2019

Choose a reason for hiding this comment

jwasinger Apr 6, 2019

Choose a reason for hiding this comment

twinstar26 Apr 6, 2019

Choose a reason for hiding this comment

twinstar26 commented Apr 7, 2019

gumb0 left a comment

Choose a reason for hiding this comment

gumb0 Apr 8, 2019

Choose a reason for hiding this comment

gumb0 Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

gumb0 Apr 8, 2019

Choose a reason for hiding this comment

twinstar26 commented Apr 8, 2019

gumb0 commented Apr 8, 2019

twinstar26 commented Apr 8, 2019

twinstar26 commented Apr 9, 2019 • edited Loading

gumb0 commented Apr 9, 2019

gumb0 left a comment

Choose a reason for hiding this comment

gumb0 commented Apr 9, 2019

gumb0 commented Apr 9, 2019

twinstar26 commented Apr 9, 2019

twinstar26 commented Apr 9, 2019

gumb0 commented Apr 9, 2019

twinstar26 commented Apr 5, 2019 •

edited

Loading

codecov-io commented Apr 6, 2019 •

edited

Loading

gumb0 Apr 8, 2019 •

edited

Loading

twinstar26 commented Apr 9, 2019 •

edited

Loading