Page MenuHomeWildfire Games

Consistently use Hexify to convert u8* to hex strings
ClosedPublic

Authored by elexis on Jun 29 2018, 9:04 AM.

Details

Summary

There are as many different implementations to convert a hash sum to a hexadecimal string as there are occurrences thereof.
Using the Hexify function we minimize the code in use and avoid using the platform dependent sprintf.
This also addresses the code style concern to JSInterface_Main.cpp in rP21850.

Test Plan

The hashs must remain the same, in particular it is relevant to pass the correct arraysize everywhere.
If the CacheLoader string differs, you see all textures being reloaded.
If the ModIO string differs, you see a hash mismatch when downloading mods.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Jun 29 2018, 9:04 AM
Vulcan added a subscriber: Vulcan.Jun 29 2018, 9:15 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...

source/maths/tests/test_MD5.h
|  36| »   »   m.Update((const·u8*)input,·strlen(input));
|    | [MAJOR] CPPCheckBear (nullPointer):
|    | Possible null pointer dereference: input
Executing section JS...

Link to build: https://jenkins.wildfiregames.com/job/differential/670/display/redirect

There is many duplication of std::string(reinterpret_cast<...>...), especially for reinterpret_cast it's not very good. I suggest to add a function Hexify(const u8* data, size_t size) to fix the number of reinterpret_cast.

Also it'd be better to use const char instead of char const to be consistent with other code (they're actually equal, but it's for the same style).

The Hexify can be refactored later, especially ugly (int)(unsigned char).

elexis updated this revision to Diff 6814.Jul 22 2018, 5:44 PM

Introduce a second function to remove conversions

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Default...
Executing section Source...

source/maths/tests/test_MD5.h
|  36| »   »   m.Update((const·u8*)input,·strlen(input));
|    | [MAJOR] CPPCheckBear (nullPointer):
|    | Possible null pointer dereference: input
Executing section JS...

Link to build: https://jenkins.wildfiregames.com/job/differential/685/display/redirect

forgot to upload Util.cpp and don't want to revert now:

std::string Hexify(const u8* s, size_t length)
{
	std::stringstream str;
	str << std::hex;
	for (size_t i = 0; i < length; ++i)
		str << std::setfill('0') << std::setw(2) << (int)s[i];
	return str.str();
}
In D1591#64047, @elexis wrote:

forgot to upload Util.cpp and don't want to revert now:

std::string Hexify(const u8* s, size_t length)
{
	std::stringstream str;
	str << std::hex;
	for (size_t i = 0; i < length; ++i)
		str << std::setfill('0') << std::setw(2) << (int)s[i];
	return str.str();
}
str << std::setfill('0') << std::setw(2) << static_cast<int>(s[i]);
elexis added a comment.Aug 5 2018, 7:32 PM

Here the patch with debug output that I used to compare previous and new hash

mod.io download and lobby login still work, so their hashes are the same, at least on this platform.

There is a double c-style cast in the other Hexify function of Util.cpp then:

 	for (const char& c : s)
		str << std::setfill('0') << std::setw(2) << (int)(unsigned char)c;

According to https://en.cppreference.com/w/cpp/language/explicit_cast and my compiler this translates to 2x static_cast.

The first cast is necessary because char may be signed or unsigned. If it's signed and converted to int, it will be negative if the representation of the char is in the lower half of the value range.
The second cast is necessary because the << operator only supports few arithmetic types, int is one of them and the implicit cast is avoided.
http://www.cplusplus.com/reference/ostream/ostream/operator%3C%3C/

So str << std::setfill('0') << std::setw(2) << static_cast<signed int>(static_cast<unsigned char>(c)); it is.

That the result is the same can be tested using non-visual replay.

source/ps/CacheLoader.cpp
143 ↗(On Diff #6814)

See this 8? Notice it's only half of DIGESTSIZE. This shortened the filename by dropping half of the hash, which leads to more (still nearly impossible) collisions.

Previously: cache/public/art/textures/ui/pregame/backgrounds/hellenes1-2.png.f587cdc3d844b799.dds
Newly: cache/public/art/textures/ui/pregame/backgrounds/hellenes1-2.png.f587cdc3d844b799fd4597e731b1c964.dds

It is often recommended that windows paths should not be longer than 260 chars, I'm unconvinced that saving these 8 characters is a meaningful measure to stay below that number.

Notice the change means that all svn .cache files are now obsolete and have to be regenerated.

elexis updated this revision to Diff 6843.Aug 5 2018, 7:37 PM

Add forgotton Util.cpp diff, static_casts for the other Hexify function, 2018, tested everything again

Vulcan added a comment.Aug 5 2018, 7:45 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...

source/maths/tests/test_MD5.h
|  36| »   »   m.Update((const·u8*)input,·strlen(input));
|    | [MAJOR] CPPCheckBear (nullPointer):
|    | Possible null pointer dereference: input
Executing section JS...

Link to build: https://jenkins.wildfiregames.com/job/differential/711/display/redirect

In D1591#64287, @elexis wrote:

There is a double c-style cast in the other Hexify function of Util.cpp then:

Yes, I mentioned this some time ago.

According to https://en.cppreference.com/w/cpp/language/explicit_cast and my compiler this translates to 2x static_cast.
The first cast is necessary because char may be signed or unsigned. If it's signed and converted to int, it will be negative if the representation of the char is in the lower half of the value range.
The second cast is necessary because the << operator only supports few arithmetic types, int is one of them and the implicit cast is avoided.

Actually << supports char: http://www.cplusplus.com/reference/ostream/ostream/operator-free/, but it prints a value as ASCII, not as the arithmetic one. We need these 2 casts, because casts depends on how these values are stored, especially negative values.

But it still looks weird. Probably we don't need to store the array of uint8_t as the std::string.

I see 10 calls to Hexify(string), so we need to convert these chars one way or another.

source/ps/Util.cpp
434 ↗(On Diff #6843)

-signed

vladislavbelov accepted this revision.Aug 6 2018, 10:41 PM
vladislavbelov added inline comments.
source/ps/CacheLoader.cpp
24 ↗(On Diff #6843)

Alphabetic order.

138 ↗(On Diff #6843)

The comment should be removed then.

This revision is now accepted and ready to land.Aug 6 2018, 10:41 PM
This revision was automatically updated to reflect the committed changes.