Paths

Table of Contentst

Diffusion 0 A.D. Public Repository rP21850

Persist the lobby Terms Of Use and Terms Of Service checkbox if the logged in…
AuditedrP21850
Actions

Description

Persist the lobby Terms Of Use and Terms Of Service checkbox if the logged in user and the accepted versions of the pages didn't change since last login, refs #5218.

This way the user is only forced to read the Terms again that changed or if the user logged in from a different machine.
Use md5sum since it is sufficiently resistant against collisions and doesn't freeze the window for 2 seconds like EncryptPassword / SHA256 does, refs #4399.
Use 0 instead of empty string in default.cfg, refs #3990.

Differential Revision: https://code.wildfiregames.com/D1575
Partial review by: Vladislav

Details

Auditors

vladislavbelov

Committed

elexis

Jun 21 2018, 6:38 PM

Differential Revision

D1575: Persist lobby terms agreement checkbox if the terms didn't change

Parents

rP21849: Fix XML syntax error in rP21847 reported by gameboy.

Branches

Unknown

Tags

Unknown

Build Status

Buildable 6261
Build 10392: Trigger Windows Autobuild
Build 10390: Post-Commit Build	Jenkins

Event Timeline

elexis mentioned this in rP21851: Fix conversion in previous rP21850 / D1575..Jun 21 2018, 7:27 PM

elexis committed rP21850: Persist the lobby Terms Of Use and Terms Of Service checkbox if the logged in….Jun 21 2018, 6:38 PM

vladislavbelov raised a concern with this commit.Jun 21 2018, 8:12 PM

vladislavbelov added a subscriber: vladislavbelov.

vladislavbelov added inline comments.

/ps/trunk/source/ps/scripting/JSInterface_Main.cpp
121	Own worse implementation instead of the duplication of the more accurate one.

This commit now has outstanding concerns.Jun 21 2018, 8:12 PM

elexis added inline comments.Jun 21 2018, 8:14 PM

/ps/trunk/source/ps/scripting/JSInterface_Main.cpp
121	Which one?

Harbormaster failed to build B6261: rP21850: Persist the lobby Terms Of Use and Terms Of Service checkbox if the logged in…!Jun 21 2018, 8:48 PM

vladislavbelov added inline comments.Jun 21 2018, 8:49 PM

/ps/trunk/source/ps/scripting/JSInterface_Main.cpp

121

I already mentioned it 2 times, in JSInterface_Lobby.cpp:

	static const char base16[] = "0123456789ABCDEF";
	char hex[2 * DIGESTSIZE];
	for (int i = 0; i < DIGESTSIZE; ++i)
	{
		hex[i*2] = base16[encrypted[i] >> 4];		// 4 high bits
		hex[i*2 + 1] = base16[encrypted[i] & 0x0F];	// 4 low bits
	}
	return std::string(hex, sizeof(hex));

elexis added inline comments.Jun 21 2018, 8:56 PM

/ps/trunk/source/ps/scripting/JSInterface_Main.cpp
121	I thought you wanted the entire function (which is why I test that SHA256 function and found it was too slow.) Why would that one be better? "0123456789ABCDEF" seems an ugly hardcoding to me. We should specify the format, not reimplement the format which also ends up in less code (also doesn't need that conversion at the end).

vladislavbelov added inline comments.Jun 21 2018, 9:08 PM

/ps/trunk/source/ps/scripting/JSInterface_Main.cpp
121	As I said on IRC, `sprintf_s` is more ugly than `base16`. Because it writes `\0` for each byte, it requires `+1` size, you need to guarantee, that it writes exactly one `\0` symbol. You don't check, that it's really wrote all string, so it's UB in case it didn't write the `\0` symbol.

The code comes from the MD5 test file which is tested on a number of platforms since years.
(If there was an explanation as to when how this could break, the UserReporter sprintf may also be considered.)

static const char base16[] = "0123456789ABCDEF";

I'd consider this the hardcoding anti-pattern, so I don't really share the opion that that piece of code is nicer.

I can change it, but then for a good reason.

This commit now requires verification by auditors.Jun 22 2018, 1:15 PM

In rP21850#30995, @elexis wrote:

The code comes from the MD5 test file which is tested on a number of platforms since years.
(If there was an explanation as to when how this could break, the UserReporter sprintf may also be considered.)

static const char base16[] = "0123456789ABCDEF";

I'd consider this the hardcoding anti-pattern, so I don't really share the opion that that piece of code is nicer.

I can change it, but then for a good reason.

You have 2 implementation depended hardcoded numbers, it's more than 1. Also you get it from the test code, instead of the production code. It's not good.

std::stringstream - no hardcoded things.

This commit now has outstanding concerns.Jun 22 2018, 4:42 PM

vladislavbelov added inline comments.Jun 22 2018, 5:12 PM

/ps/trunk/source/ps/scripting/JSInterface_Main.cpp
125	This line is insecure too.

You have 2 implementation depended hardcoded numbers, it's more than 1.

0123456789ABCDEF is not only one hardcoding, it's hardcoding of 16 different pieces of information that can fail individually, can be and are abstracted by the sprintf.

It's not good.

Your arguments claiming this to be a defect aren't.

Also you get it from the test code, instead of the production code.

To claim that test code that is run since 8 years rP7269 and run in 52 other places being something that isn't tested well while the mod.io code from few weeks ago is seems far out to me.

Claiming that the null-terminator can be absent is something that you should have witnessed somewhere or can demonstrate before claiming a defect. sprintf_s is used in 53 places in the current engine and we would have noticed if it wasn't null-terminated.
It even was noticed on android, evident by the macro testing for android.
The specs also state that \0 is written
http://www.cplusplus.com/reference/cwchar/vswprintf/
http://www.cplusplus.com/reference/cstdio/vsnprintf/

So there is reasonable belief that it is written on all platforms that we currently support.

So if your claimed defect is true, then I at least have no indication of any claim making this a reproducible assumption.

Notice that the hash may not depend on null-terminating character, because all platforms should compute the same md5sum per given data.
So if there are platforms not printing the null-termination character, then this can't be ignored by hashing undefined data using a stringstream.

I do agree with the point that stringstream removes the information that there is a null-terminating character, but this isn't a defect.
If this is only personal preference, I would be more inclined to change this if you would ask friendly and say please rather than claiming this to be an intersubjectively perceivable defect.

Since this isn't the first time that this occured and my words didn't change, I can act differently to demonstrate how to formally file complaints that aren't founded in reproducible arguments.

You also never posted anything about the JS code and I suspect you never tried it or read it, yet you set yourself as a reviewer for this time-critical task by requesting changes, which is something I obejct to.

/ps/trunk/source/ps/scripting/JSInterface_Main.cpp
125	If digestr is null-terminated, then this line is secure: http://www.cplusplus.com/reference/string/string/string/ That digestr is null-terminated is tested.

This commit now requires verification by auditors.Jun 22 2018, 6:11 PM

In rP21850#31004, @elexis wrote:

0123456789ABCDEF is not only one hardcoding, it's hardcoding of 16 different pieces of information that can fail individually, can be and are abstracted by the sprintf.

It's not different 16 pieces, it's logically related pieces. You said you don't want to add hardcoded things, but you did add it. Without review. With my Needs changes. You have std::stringstream without so hardcoded numbers, but you didn't use it.

It's not good.

Your arguments claiming this to be a defect aren't.

I don't understand what do you mean here.

Also you get it from the test code, instead of the production code.

To claim that test code that is run since 8 years rP7269 and run in 52 other places being something that isn't tested well while the mod.io code from few weeks ago is seems far out to me.

Again, the test code wan't run so many times as the production one.

Claiming that the null-terminator can be absent is something that you should have witnessed somewhere or can demonstrate before claiming a defect. sprintf_s is used in 53 places in the current engine and we would have noticed if it wasn't null-terminated.
It even was noticed on android, evident by the macro testing for android.
The specs also state that \0 is written
http://www.cplusplus.com/reference/cwchar/vswprintf/
http://www.cplusplus.com/reference/cstdio/vsnprintf/
So there is reasonable belief that it is written on all platforms that we currently support.

Why did you post standard only for one platform? sprintf_s is platform depended define.

So if your claimed defect is true, then I at least have no indication of any claim making this a reproducible assumption.

If it's not fired yet, it's not the reason to add new ugly code.

I would be more inclined to change this if you would ask friendly and say please rather than claiming this to be an intersubjectively perceivable defect.

As I mentioned above. I did it. I said it in the diff and mark it as Needs Changes, but you did ignore it. So the problem isn't on my side.

Since this isn't the first time that this occured and my words didn't change, I can act differently to demonstrate how to formally file complaints that aren't founded in reproducible arguments.

So in this case I have rights to revert this commit (because C++). But it won't help much. Because it'll be like in the chromium project: revert of revert of revert ....

You also never posted anything about the JS code and I suspect you never tried it or read it, yet you set yourself as a reviewer for this time-critical task by requesting changes, which is something I obejct to.

Because I'm worrying about C++ code. Because less people understand what's going here.

I disagree with C++ changes in this revision, because it adds new ugly code, instead of duplication more tested and already existed in the production (not tests) part.

/ps/trunk/source/ps/scripting/JSInterface_Main.cpp
125	Nope, it's null-terminated only if the `sprintf_s` didn't fail.

This commit now has outstanding concerns.Jun 22 2018, 6:32 PM

Most if not all of the 60+ sprintf_s calls in both production and test code rely on the null-terminator.
So if you assume the null-terminator not being written in sprintf_s being a release blocker, then we have to fix 60+ occurrences or write a new wrapper.

I don't see the criticism that isn't founded in personal preference of code style.
I can use a stringstream voluntarily to please your personal preference of code style and perhaps even become a subscriber to that code style if the imposition ceased.
But I won't use a stringstream to fix a defect in this revision unless the claimed defect is substantiated against the indication.

In rP21850#31025, @elexis wrote:

I don't see the criticism that isn't founded in personal preference of code style.
I can use a stringstream voluntarily to please your personal preference of code style and perhaps even become a subscriber to that code style if the imposition ceased.
But I won't use a stringstream to fix a defect in this revision unless the claimed defect is substantiated against the indication.

The problem is that you used the code from tests. The problem is that you didn't see difference between single write in a buffer and multiple and have insecure construction of std::string. The problem is that you didn't wait my approve or full review.

If you think that it's only my personal preferences, then you'd need more experience in C++.

insecure construction of std::string

If you mentioned a hypothesis other than platform depending missing null-termination in sprintf_s that can cause the function to not return the md5 hash or behave unintentionally, then I didn't comprehend it.

In rP21850#31027, @elexis wrote:

insecure construction of std::string

If you mentioned a hypothesis other than platform depending missing null-termination in sprintf_s that can cause the function to not return the md5 hash or behave unintentionally, then I didn't comprehend it.

If it works now it's not the reason to add a non-standard workaround. But do you agree that it was wrong to commit it without this discussion in the diff (especially without reviews)?

In rP21850#31028, @vladislavbelov wrote:

In rP21850#31027, @elexis wrote:

insecure construction of std::string

If you mentioned a hypothesis other than platform depending missing null-termination in sprintf_s that can cause the function to not return the md5 hash or behave unintentionally, then I didn't comprehend it.

If it works now it's not the reason to add a non-standard workaround.

A sprintf that we have in 60 places may not be ideal but is also not a workaround.

But do you agree that it was wrong to commit it without this discussion in the diff (especially without reviews)?

But do you agree that it was wrong to commit it without
this discussion in the diff

As I have stated I didn't read or comprehend the sprintf argument from the revision proposal, otherwise I woul have likely applied it like all the other of your comments that I did apply.

especially without reviews

Yes I still disagree with the principle of making reviews mandatory unless someone pays us for doing so.

In rP21850#31029, @elexis wrote:

A sprintf that we have in 60 places may not be ideal but is also not a workaround.

I didn't tell about sprintf_s at all in the last sentence, but about modifying the same buffer with hardcoded offsets.

Yes I still disagree with the principle of making reviews mandatory unless someone pays us for doing so.

I think we need to discuss with the team (@Itms), before do such dangerous things. I don't look on JS changes. But for C++ you may introduce a critical error (in a worst case you may damage the end user platform) without experience in some theme of it.

UPD. I did a partial review of it, but you did ignore it for some reason.

In rP21850#31031, @vladislavbelov wrote:

In rP21850#31029, @elexis wrote:

A sprintf that we have in 60 places may not be ideal but is also not a workaround.

I didn't tell about sprintf_s at all in the last sentence, but about modifying the same buffer with hardcoded offsets.

and is modifying the same buffer with hardcoded offsets something relating to defect or aesthetics and good practice?
I'm not saying the hardcoded offsets are the most beautiful variant, I'm saying that I don't see a defect if the null termination character is written and I see indication that the null-termination character is written on all platforms used in the last decade.

I don't look on JS changes. But for C++ you may introduce a critical error

Critical JS or XML errors can be as compromising.

UPD.

What's that?

I did a partial review of it, but you did ignore it for some reason.

I did a answer all of the points you have posted in D1575:

No UNUSED in the header - applied
static_cast, applied
It's a duplication of code (and not really good). Use the implementation from JSInterface_Lobby.cpp

a) The argument was duplication, not anything about safety and I answered the duplication.
b) That you were refering to string construction is not visible from the text which you have posted, nor from the line number.
The comment posted there can and was confused with a recommendation to use SHA256:

Use the implementation from JSInterface_Lobby.cpp
We have SHA256 in the mentioned JSInterface_Lobby.cpp.

Ambiguities must be reduced where possible if one wants to be understood.

Yes I still disagree with the principle of making reviews mandatory

I think we need to discuss with the team (@Itms), before do such dangerous things.

In the average case in 2016 it stalled development and didn't actually improve code quality.
In 2017 I stopped trying to fulfil this guideline because I could get things done in 1 year that took me 3 months without the guideline.
But even in the best case it consumes an order of magnitude more time to progress.
Writing a law that code quality must be good doesn't actually make it so, just like having writen a guideline that a review must be performed well doesn't actually mean that the reviewer read the patch.
Also is this an argumentum ad populum (argumentum ad verecundiam)?

If there is a relevant platform that doesn't write the null-terminator

then we have to fix many places, not only this one.
then a lot of production and testing code is broken since a decade, but we didn't perceive that

If there is a different defect (unintended behavior), it wasn't stated.

As I have offered above, I can change this loop immediately for aesthetics and good practice if the claim of a defect (unintended behavior) is either removed or precised.

Hey I know everyone is tensed and everyone wants to re release soon. But don't you think we should stick together instead of killing each other like this ? We are AFAIK all adults now. 0 A.D. is one of the greatest achievement I have in my life. I'm really proud to be among you guys but we got to stop this right now. We can't keep tearing each other apart. You can't also keep fighting like this over text messages. Maybe you guys could go on a voice chat or something and explain to each other what you mean, empty your bags and find a common agreement. This will be faster will save you both time and will allow you to move on. I don't want this release to be an A16 again we lost too much things already. It's the greatest release ever and still from a team point of view the worst we had.

So please I beg you find a way to pull this through. Take one hour discuss things like friends and not like hurt people. I know you can do this.

In rP21850#31034, @elexis wrote:

UPD.

What's that?

I edited the comment few minits later (UPD = Update).

In rP21850#31035, @Stan wrote:

Hey I know everyone is tensed and everyone wants to re release soon. But don't you think we should stick together instead of killing each other like this ? We are AFAIK all adults now. 0 A.D. is one of the greatest achievement I have in my life. I'm really proud to be among you guys but we got to stop this right now. We can't keep tearing each other apart. You can't also keep fighting like this over text messages. Maybe you guys could go on a voice chat or something and explain to each other what you mean, empty your bags and find a common agreement. This will be faster will save you both time and will allow you to move on. I don't want this release to be an A16 again we lost too much things already. It's the greatest release ever and still from a team point of view the worst we had.

So please I beg you find a way to pull this through. Take one hour discuss things like friends and not like hurt people. I know you can do this.

We'll discuss in PM.

elexis mentioned this in D1591: Consistently use Hexify to convert u8* to hex strings.Jun 29 2018, 9:04 AM

I agree that it's good practice to not use functions that have to be maintained and might break with every new untested platform.
I also agree that relying on platform-dependent maintenance where there is a feasible platform-independent alternative can be considered a defect.

I commit D1591 now because you like the patch and so as to get rid of the patch. But:

What I disagree with is that this commit could demand a rerelease and that this code would not have been tested in production code.

Most if not all functions utilizing sprintf_s require null-termination:
JSI_Debug::GetBuildTimestamp, CProfileNodeTable::GetCellText, ps_generate_guid, CRendererStatsTable::GetCellText, CUserReporter::LoadUserID, MessagePasserImpl::MessagePasserImpl(), ...

So if this commit is worth a concern, the other calls should be of concern too, a ticket should document this.

If this diff requires a fix prior to the re-release because it might break on a new platform, replacing the other sprintf_s functions should be a release-blocker as well.
Do we want that? If we say the other 60 function calls can be improved later then this one is of no urgency either.

elexis requested verification of this commit.Aug 8 2018, 2:59 PM

This commit now requires verification by auditors.Aug 8 2018, 2:59 PM

elexis mentioned this in rP21863: Unify duplicate u8* to hex string functions in a new Hexify function variant….Aug 8 2018, 3:01 PM

vladislavbelov accepted this commit.Aug 8 2018, 10:25 PM

All concerns with this commit have now been addressed.Aug 8 2018, 10:25 PM

elexis mentioned this in rP21872: Fix missing alias in rP21850 / D1575, refs #5218, taken from D1590..Aug 23 2018, 7:26 PM