Info code refactoring #604

theAsmodai · 2018-04-16T23:33:10Z

I have made a lot of changes and hope all potencial bugs was fixed. Waiting for reviews.

Removed engine initialization in info tests

Fix duplicate info tests

WPMGPRoSToTeMa · 2018-04-17T03:24:45Z

sv_rehlds_userinfo_transmitted_fields default value meaning changed. Can this break some games?

theAsmodai · 2018-04-17T10:06:51Z

I'm added default keys used in Half-Life. Don't know any mods that using custom keys. If you think rehlds should transmit all fields by default, how would you suggest to implement it?

LevShisterov

Checked only half, will look other part a bit later.

LevShisterov · 2018-04-17T09:41:32Z

rehlds/HLTV/common/InfoString.cpp

@@ -81,7 +81,7 @@ bool InfoString::SetString(char *string)
 		return false;
 	}

-	Q_strnlcpy(m_String, string, m_MaxSize);
+	Q_strnlcpy(m_String, string, m_MaxSize - 1);


This looks wrong, Q_strnlcpy do n-1 inside.

LevShisterov · 2018-04-17T09:41:38Z

rehlds/HLTV/common/InfoString.cpp

@@ -95,7 +95,7 @@ void InfoString::SetMaxSize(unsigned int maxSize)
 	if (m_String)
 	{
 		if (maxSize > Q_strlen(m_String)) {
-			Q_strnlcpy(newBuffer, m_String, maxSize);
+			Q_strnlcpy(newBuffer, m_String, maxSize - 1);


Same as above.

LevShisterov · 2018-04-17T09:49:04Z

rehlds/engine/host_cmd.cpp

+			s++;
+		}
+
+		*s = '0';


Should be '\0'. May be because of this pure =0 is better? Why testing didn't catched this one?

LevShisterov · 2018-04-17T09:55:35Z

rehlds/engine/info.cpp

 // Searches the string for the given
 // key and returns the associated value, or an empty string.
-const char* EXT_FUNC Info_ValueForKey(const char *s, const char *key)
+const char* Info_ValueForKey(const char *s, const char *lookup)


EXT_FUNC removal, probably ok, just curious, will be fine?

Checked again, it used in pmove, not only in engfuncs via proxy.

LevShisterov · 2018-04-17T10:03:52Z

rehlds/engine/info.cpp

+
+		// skip key
+		while (*s != '\\')
+			s++;


Can get past end of the string.

I proceeded from the fact that buffer has already been checked for correctness. But can add for better reliability.

WPMGPRoSToTeMa · 2018-04-17T10:41:06Z

how would you suggest to implement it?

Something of these:

sv_rehlds_userinfo_transmitted_fields "all"
sv_rehlds_userinfo_transmitted_fields "compat"
sv_rehlds_userinfo_transmitted_fields "legacy"
sv_rehlds_userinfo_transmitted_fields "old"
sv_rehlds_userinfo_transmitted_fields "\..."
sv_rehlds_userinfo_transmitted_fields "\all..."
sv_rehlds_userinfo_transmitted_fields "\...all"

LevShisterov · 2018-04-17T10:53:50Z

rehlds/public/strtools.h

-	Q_strncpy(dest, src, n - 1);
-	dest[n - 1] = '\0';
+	Q_strncpy(dest, src, n);
+	dest[n] = '\0';


This is, probably, not a good idea, because function goes beoynd specified size. You can just add description to a function, that it is copy n-1 chars and adds a newline in the end.

I consider this way is more logical. All functions of this family takes maximum string length, not the buffer size.

https://ru.wikipedia.org/wiki/Strlcpy

May be use this one naming/signature?

LevShisterov · 2018-04-17T11:03:55Z

sv_rehlds_userinfo_transmitted_fields - empty value should work as before. So in case mod needs some custom info fields, server operator can set empty value for the cvar.

theAsmodai · 2018-04-17T11:28:40Z

I would like to add protection against wrong usage of sv_rehlds_userinfo_transmitted_fields. In my version empty value mean that only critical engine fields will be transmitted. So, cstrike owners can remove topcolor and bottomcolor since they are not used. Maybe we can add an additional cvar to on/off this option?

IgnacioFDM · 2018-04-18T02:16:50Z

I'd recommend not breaking compatibility with sv_rehlds_userinfo_transmitted_fields

LevShisterov · 2018-04-18T17:29:04Z

rehlds/engine/info.cpp

+			s++;
+
+		if (keyLen < lookupLen)
+			continue;


I think Info_RemoveKey should remove exact keys, here for example and here (shouldn't remove password_test too). Possibly write a test for this also?

I'm copied the default engine behaviour. But since duplicate keys will be checked, we can change this to exact match.

You are right, it was so before. I think we should change this.

LevShisterov · 2018-04-18T17:45:03Z

rehlds/engine/info.cpp

+
+	for (auto& field : g_info_important_fields) {
+		if (!Q_strncmp(key, field.name, keyLen))
+			return true;


Same here, "top" will be considered as important.

Yes, you are right. I'm forgot to recheck it.

LevShisterov · 2018-04-18T18:16:44Z

rehlds/engine/info.cpp

+	{
+		if (key[0] == '_') {
+			Con_Printf("%s: private keys couldn't be transmitted.\n", __FUNCTION__);
+			continue;


It will spam in console each time. Would be good to output key name at least.

LevShisterov · 2018-04-18T18:17:22Z

rehlds/engine/info.cpp


+		if (Q_strlen(key) >= MAX_KV_LEN) {
+			Con_Printf("%s: keys and values must be < %i characters\n", __FUNCTION__, MAX_KV_LEN);
+			continue;


LevShisterov · 2018-04-18T18:26:24Z

rehlds/engine/info.cpp

+	char add[512], valueBuf[32];
+	size_t userInfoLength = 0;
+
+	for (auto field : g_info_transmitted_fields)


Can I suggest to rename it to g_info_transmit_fields?

sv_rehlds_userinfo_transmitted_fields probably should be renamed too.

LevShisterov · 2018-04-18T18:36:16Z

About sv_rehlds_userinfo_transmitted_fields cvar and g_info_important_fields, I have a word from a side that some server ops wish to block *hltv and *sid sending to hide steamids, etc. So, probably mark these as not forced and include in default value?

WPMGPRoSToTeMa · 2018-04-18T20:20:58Z

May be model should also be blockable due to #342.

theAsmodai · 2018-04-18T20:21:30Z

About sv_rehlds_userinfo_transmitted_fields cvar and g_info_important_fields, I have a word from a side that some server ops wish to block *hltv and *sid sending to hide steamids, etc. So, probably mark these as not forced and include in default value?

I think it's a bad idea. *sid blocking will break avatars showing on clients. And anyway hiding of steamid is a bad idea because it was designed as public identificator of players.

theAsmodai · 2018-04-18T20:23:53Z

@WPMGPRoSToTeMa, good idea, but this can lead to problems when used improperly.

IgnacioFDM · 2018-04-18T21:23:44Z

@WPMGPRoSToTeMa

That's very useful. I'm in fact using that feature (hardcoded blocking of model) in my personal branch

LevShisterov · 2018-04-18T21:39:06Z

*sid blocking will break avatars showing on clients.

Yes, this is the desired (not by me) effect.

anyway hiding of steamid is a bad idea because it was designed as public identificator of players.

I am also not welcome the idea of blocking status and other steamid sources. But, still, we are open source, why not let people to shoot in the leg? Anyway, it's up to you, I don't care about this.

LevShisterov · 2018-04-23T15:50:14Z

rehlds/engine/info.cpp

 qboolean Info_IsKeyImportant(const char *key)
 {
 	if (key[0] == '*')
 		return true;
+
+	for (auto& field : g_info_important_fields) {
+		if (!Q_strcmp(key, field.name))


It is possible to just do a char comparision at the end of the key for a '\' after a successfull Q_strncmp (in previous variant of that code). Then you woudn't do a copy of the key. Also, you can prefill g_info_important_fields with key lengths and then just do a len comparision before Q_strncmp. But I am ok with current variant too, so at your discretion.

LevShisterov · 2018-04-23T16:07:56Z

rehlds/engine/info.cpp

+
+		existingKeys[existingKeysNum].start = key;
+		existingKeys[existingKeysNum].len = keyLen;
+		existingKeysNum++;


Ah, one more thing: you should somehow assure that you will not go past existingKeys border. I think it should be incoming buffer length checking at the function start.

Ok. And the last question: should sv_rehlds_userinfo_transmitted_fields be renamed or not? I'm created wiki page https://github.com/dreamstalker/rehlds/wiki/Userinfo-keys and later I will will set the correct default value and description for this cvar.

Thought about this, probably non-default value is used often in CS, so we can leave it as it is.

…tted_fields

theAsmodai · 2018-04-23T23:14:00Z

Tests was fixed.

theAsmodai added 11 commits April 16, 2018 22:25

info.cpp refactoring

33be24c

Add more sanity checks

b07cfcf

Fix compilation errors

b78cb88

Update readme

81576fb

Update info unittests

c0240d6

Fix errors

9755547

Update tests data set

29780d5

Fixed setstarkey function and tests

90c9b60

Added test InfoCollectFields

6ff6119

Removed engine initialization in info tests

Fixed bug with some duplicate info fields

4db5a8a

Change order of keys in test result

a4767ef

Fix duplicate info tests

theAsmodai requested review from LevShisterov and s1lentq April 16, 2018 23:33

LevShisterov requested changes Apr 17, 2018

View reviewed changes

LevShisterov reviewed Apr 17, 2018

View reviewed changes

theAsmodai force-pushed the info_refactoring branch from 467eb48 to bdbb0c7 Compare April 17, 2018 12:53

Changes requested by LevShisterov

f6dad3b

theAsmodai force-pushed the info_refactoring branch from bdbb0c7 to f6dad3b Compare April 17, 2018 13:01

Add missed prefixes

2a4232a

LevShisterov requested changes Apr 18, 2018

View reviewed changes

theAsmodai added 2 commits April 23, 2018 00:45

Don't force fields, fixed bugs

6be72ec

Update tests data

3fd745f

LevShisterov reviewed Apr 23, 2018

View reviewed changes

LevShisterov approved these changes Apr 23, 2018

View reviewed changes

LevShisterov reviewed Apr 23, 2018

View reviewed changes

theAsmodai force-pushed the info_refactoring branch 2 times, most recently from ce3b7eb to 60b201e Compare April 23, 2018 16:13

Make _vgui_menus important and fix tests passing

625b962

theAsmodai force-pushed the info_refactoring branch from 60b201e to 625b962 Compare April 23, 2018 16:19

theAsmodai added 2 commits April 23, 2018 21:35

Reverted back behavior and description for sv_rehlds_userinfo_transmi…

ce47bd2

…tted_fields

Check for overflow of existingKeys[]

2b3e0c2

theAsmodai force-pushed the info_refactoring branch 3 times, most recently from c3027e9 to fca78f0 Compare April 23, 2018 21:35

Fixed test for default sv_rehlds_userinfo_transmitted_fields value

3294ecf

theAsmodai force-pushed the info_refactoring branch from fca78f0 to 3294ecf Compare April 23, 2018 21:56

s1lentq approved these changes May 4, 2018

View reviewed changes

theAsmodai merged commit f324df8 into master May 4, 2018

theAsmodai deleted the info_refactoring branch May 4, 2018 20:38

Info code refactoring #604

Info code refactoring #604

Conversation

theAsmodai commented Apr 16, 2018

WPMGPRoSToTeMa commented Apr 17, 2018

theAsmodai commented Apr 17, 2018

LevShisterov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WPMGPRoSToTeMa commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LevShisterov commented Apr 17, 2018

theAsmodai commented Apr 17, 2018

IgnacioFDM commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LevShisterov commented Apr 18, 2018 • edited Loading

WPMGPRoSToTeMa commented Apr 18, 2018

theAsmodai commented Apr 18, 2018 • edited Loading

theAsmodai commented Apr 18, 2018

IgnacioFDM commented Apr 18, 2018

LevShisterov commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theAsmodai Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theAsmodai commented Apr 23, 2018

LevShisterov commented Apr 18, 2018 •

edited

Loading

theAsmodai commented Apr 18, 2018 •

edited

Loading

theAsmodai Apr 23, 2018 •

edited

Loading