Fix Teeworlds server query, #3 #4

illwieckz · 2014-11-16T08:18:24Z

Hi, this fixes the Teeworlds server query, it tries with the 3 known getinfo packets.

I renamed the -tee argument to -tees since I will work next on the -teem Teeworlds master query.

Since Teeworlds support was broken since many years, there is probably nobody that uses the -tee arg today.

stevenh · 2014-11-16T19:06:42Z

qstat.cfg

@@ -511,3 +511,4 @@ gametype ALIENARENAM new extend Q2M
    master query = empty full
    master for gametype = ALIENARENAS
 end
+


irelavent change, please remove from the commit.

stevenh · 2014-11-16T19:09:23Z

Overall can you please update the formatting for the teeworld module as per the new style, which is based off:
https://www.freebsd.org/cgi/man.cgi?query=style&sektion=9

An example of this can be found in the dirty bomb module.

illwieckz · 2014-11-16T19:53:02Z

-query_status_t deal_with_tee_packet( struct qserver *server, char *rawpkt, int pktlen )
+query_status_t deal_with_tee_packet( struct qserver *server, char *pkt, int pktlen )

This is still a raw packet please keep the old var name

That was because

-query_status_t deal_with_tee_packet( struct qserver *server, char *rawpkt, int pktlen )
+query_status_t deal_with_tee_packet( struct qserver *server, char *pkt, int pktlen )
-       char *pkt = rawpkt + 6;

Because the + 6 was not relevant anymore, I've only rename rawpkt this line to avoid renaming each occurrence in the whole file. So, I will rename pkt as rawpkt in the whole file.

Overall can you please update the formatting for the teeworld module as per the new style, which is based off:
https://www.freebsd.org/cgi/man.cgi?query=style&sektion=9
An example of this can be found in the dirty bomb module.

OK. Good idea.

+
irelavent change, please remove from the commit.

Yes, unfortunately, I see that pointless thing after my pull request so I did not rewrite the commit to not pollute the git history and the github history (because the commit name contains the issue number, each commit rewrite can appear in the issue #3 thread).

I've already deleted this useless line, so it will disappear in my next commit (master server).

I will propose a clean pull request with server support fix + new master server when master support will be finished.

The master server support is well underway, but I need a little help, cf. #5 😉

leepa · 2014-11-16T21:49:32Z

You can delete the line and commit --amend to update your pull request.

illwieckz · 2014-11-16T22:50:24Z

I rewritten the commit.

stevenh · 2014-11-16T23:16:30Z

tee.c

+		}
+
+		/* version */
+		version = strdup (rawpkt); rawpkt += strlen (rawpkt) + 1;


Each line should be on a separate new line

I agree with that, I have done that for only one reason: mimic the existing code because it is a prudent way to do what others expect.

stevenh · 2014-11-17T12:12:21Z

tee.c

+
+	server->protocol_version = 0;
+
+	if (NULL == (tok = strtok(version, "."))) return -1;


returns should be on a new line and NULL at the other side

You're right, also, -1 has a name, SYS_ERROR.

My big question is "how this code was accepted before?" 😉

That should return PKT_ERROR

This code is worse than I imagined. 😆

stevenh · 2014-11-17T12:36:51Z

Thanks for fixing the issues, appreciate half of them are legacy code, but best to fix them as your going through :)

stevenh · 2014-11-17T14:07:19Z

This pull request is for #3

illwieckz · 2014-11-19T07:42:49Z

Hi, what do you think about renaming tee.c to teeworlds.c and qstat args to -teeworldss and -teeworldsm ? I see that newer games use complete name. Since the Teeworlds server support was broken since many years and the Teeworlds master support was never written before, there will not be many people complaining about it. 😄

stevenh · 2014-11-19T12:41:39Z

I'd keep that change separate if we did it.
On 19/11/2014 07:42, Thomas Debesse wrote:

Hi, what do you think about renaming |tee.c| to |teeworlds.c| and
qstat args to |-teeworldss| and |-teeworldsm| ? I see that newer games
use complete name. Since the Teeworlds server support was broken since
many years and the Teeworlds master support was never written before,
there will not be many people complaining about it. 😄

—
Reply to this email directly or view it on GitHub
#4 (comment).

illwieckz · 2014-11-19T13:28:24Z

OK, can you confirme it is safe to free this strdup : illwieckz@59661ba#diff-ba6f07c5675bc4ade962db2a9e368d8eR147 ?

illwieckz · 2014-12-08T03:00:49Z

Hi, we are close to do it. I think the only one thing that prevent you to merge now is my question above, if you say yes, I will modify these lines and you will be able to merge after that.

illwieckz · 2015-01-24T01:11:48Z

Hi, I've updated.

The part with the useless strdup was rewritten, and I added an extra test to verify that the whole packet had a NULL end in it. In fact, all strnlen called on packet parts are called with a safe maxlength, so this extra test is probably not necessary.

stevenh · 2015-01-24T01:23:27Z

Your strnlen tests where fine but add_rule uses strdup so if its not null terminated then "boom" ;-)

stevenh · 2015-01-24T01:45:46Z

tee.c

+	}
+
+	/* not null-terminated packet */
+	if (strnlen(rawpkt, rawpktlen) == rawpktlen && rawpkt[rawpktlen - 1] != 0) {


Why the strlen, surely that will always fail as the last byte is expected to be 0 hence the length with be rawpktlen - 1?

I wanted to test "there is no null before end" (length=maxlength) and "last is not null"

Just to verify:

$ cat a.c #include <stdio.h> #include <strings.h> int main (void) { char tab1[3]={'a', 'b', 'c'}; char tab2[3]={'a', 'b', 0}; printf("not terminated: %d\n", strnlen(tab1, 3)); printf("null terminated: %d\n", strnlen(tab2, 3)); } $ gcc -o a a.c $ ./a not terminated: 3 null terminated: 2

So, strnlen returns rawpktlen if not terminated string, first condition works and is sufficient, no need to test the last character.

So only:

if (strnlen(rawpkt, rawpktlen) == rawpktlen) { return (PKT_ERROR); }

is needed.

No you still need the null check as strnlen will return rawpktlen even if it wasn't null terminated.

stevenh · 2015-03-31T12:30:26Z

Any update on this @illwieckz ?

illwieckz · 2015-03-31T13:46:12Z

Hi @stevenh, I do not forget this, it's just that I do not have much time these days. I'll work on it soon. 😉

stevenh · 2015-03-31T14:07:02Z

No problem just checking :)

illwieckz · 2015-06-18T14:47:25Z

Just to say, I will probably be able to work on it in july or august. I’m not forgetting this, I’m just very busy. :-)

illwieckz · 2015-07-01T00:08:47Z

Hi @stevenh, I modified my patch, if you want to test it you can do that:

qstat -default tees -f /tmp/teelist.txt

With this list as /tmp/teelist.txt.

stevenh · 2015-07-01T10:53:02Z

tee.c

+	/* if inf2 or inf3 */
+	if (last_char == '2' || last_char == '3') {
+		/* token, skip */
+		current += strnlen(current, end - current) + 1; 


It looks like you could simply these tests by switching to maintaining a "remaining" instead of having to constantly check against end - current?

Hmm, I don't know why, but my question disappeared, I was asking if I can just do that:

current += strlen(current) + 1;

since I already checked that the packet is NULL-terminated.

vorot93 · 2015-09-21T19:23:53Z

This PR is nearly 1 year old. Outline any remaining issues in PR's description perhaps?

stevenh reviewed Nov 16, 2014
View reviewed changes

illwieckz force-pushed the master branch 6 times, most recently from 19d978a to d6a96f9 Compare November 16, 2014 22:31

stevenh reviewed Nov 16, 2014
View reviewed changes

illwieckz force-pushed the master branch 2 times, most recently from 7993762 to ebe44a6 Compare November 17, 2014 07:02

illwieckz mentioned this pull request Nov 17, 2014

Browse Teeworlds servers XQF/xqf#82

Closed

stevenh reviewed Nov 17, 2014
View reviewed changes

illwieckz force-pushed the master branch from ebe44a6 to 59661ba Compare November 18, 2014 18:57

illwieckz mentioned this pull request Jan 23, 2015

add Savage master support #6

Open

illwieckz changed the title ~~fix teeworld server query, #3~~ fix Teeworlds server query, #3 Jan 23, 2015

illwieckz force-pushed the master branch from 59661ba to d92f838 Compare January 24, 2015 00:28

stevenh reviewed Jan 24, 2015
View reviewed changes

fix teeworld server query, fix Unity-Technologies#3

bb0f057

illwieckz force-pushed the master branch from d92f838 to bb0f057 Compare July 1, 2015 00:06

stevenh reviewed Jul 1, 2015
View reviewed changes

illwieckz mentioned this pull request Jul 9, 2015

Tell player if their client version is different to server version Unvanquished/Unvanquished#726

Open

illwieckz changed the title ~~fix Teeworlds server query, #3~~ [WIP] fix Teeworlds server query, #3 Aug 20, 2015

stevenh changed the title ~~[WIP] fix Teeworlds server query, #3~~ Fix Teeworlds server query, #3 Nov 26, 2016

stevenh merged commit cca1ab1 into Unity-Technologies:master Nov 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Teeworlds server query, #3 #4

Fix Teeworlds server query, #3 #4

illwieckz commented Nov 16, 2014

stevenh Nov 16, 2014

stevenh commented Nov 16, 2014

illwieckz commented Nov 16, 2014

leepa commented Nov 16, 2014

illwieckz commented Nov 16, 2014

stevenh Nov 16, 2014

illwieckz Nov 17, 2014

stevenh Nov 17, 2014

illwieckz Nov 17, 2014

stevenh Nov 17, 2014

illwieckz Nov 17, 2014

stevenh commented Nov 17, 2014

stevenh commented Nov 17, 2014

illwieckz commented Nov 19, 2014

stevenh commented Nov 19, 2014

illwieckz commented Nov 19, 2014

illwieckz commented Dec 8, 2014

illwieckz commented Jan 24, 2015

stevenh commented Jan 24, 2015

stevenh Jan 24, 2015

illwieckz Jan 24, 2015

stevenh Jan 24, 2015

stevenh commented Mar 31, 2015

illwieckz commented Mar 31, 2015

stevenh commented Mar 31, 2015

illwieckz commented Jun 18, 2015

illwieckz commented Jul 1, 2015

stevenh Jul 1, 2015

illwieckz Jul 7, 2015

vorot93 commented Sep 21, 2015


		server->protocol_version = 0;

		if (NULL == (tok = strtok(version, "."))) return -1;

Fix Teeworlds server query, #3 #4

Fix Teeworlds server query, #3 #4

Conversation

illwieckz commented Nov 16, 2014

Choose a reason for hiding this comment

stevenh commented Nov 16, 2014

illwieckz commented Nov 16, 2014

leepa commented Nov 16, 2014

illwieckz commented Nov 16, 2014

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

stevenh commented Nov 17, 2014

stevenh commented Nov 17, 2014

illwieckz commented Nov 19, 2014

stevenh commented Nov 19, 2014

illwieckz commented Nov 19, 2014

illwieckz commented Dec 8, 2014

illwieckz commented Jan 24, 2015

stevenh commented Jan 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenh commented Mar 31, 2015

illwieckz commented Mar 31, 2015

stevenh commented Mar 31, 2015

illwieckz commented Jun 18, 2015

illwieckz commented Jul 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vorot93 commented Sep 21, 2015