Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch CS:GO Clantag set/get to use netprops + offset over sig + offset. #922

Merged
merged 4 commits into from Nov 9, 2018

Conversation

@Drifter321
Copy link
Member

Drifter321 commented Nov 6, 2018

So since this stupid sig keeps breaking... Here is a change to switch to grabbing a netprop address and using an offset to get the address of the clantag. Completely untested, will test if no one does prior later this week.

@busheezy busheezy mentioned this pull request Nov 6, 2018
4 of 6 tasks complete
@e54385991

This comment has been minimized.

Copy link

e54385991 commented Nov 7, 2018

L 11/07/2018 - 07:42:18: [SM] Exception reported: Failed to locate ClanTagSize offset in gamedata

@e54385991

This comment has been minimized.

Copy link

e54385991 commented Nov 7, 2018

517f7d3
Crashes when server starting

@Headline

This comment has been minimized.

Copy link
Member

Headline commented Nov 7, 2018

Call stack

windbg

thanks to sneak because he's a cutie

EDIT: I ran into #910 instead of being able to actually fix/test this one

@sneak-it

This comment has been minimized.

Copy link

sneak-it commented Nov 7, 2018

This should be it

ty @Headline 👍


if (pVar)
{
char *newValue;
pContext->LocalToString(params[2], &newValue);
Q_strncpy(*pVar, newValue, maxlen);
Q_strncpy(pVar, newValue, maxlen);

This comment has been minimized.

Copy link
@Headline

Headline Nov 7, 2018

Member

ke::SafeStrcpy ?

Copy link
Member

Headline left a comment

I think we should start using nullptr for any new accepted code

#if SOURCE_ENGINE == SE_CSGO
return SetPlayerStringVar(pContext, params, "ClanTag");
#else
static ICallWrapper *pWrapper = NULL;

This comment has been minimized.

Copy link
@Headline

Headline Nov 7, 2018

Member

NULL -> nullptr

pass[0].flags = PASSFLAG_BYVAL; \
pass[0].type = PassType_Basic; \
pass[0].size = sizeof(char *); \
pWrapper = g_pBinTools->CreateCall(addr, CallConv_ThisCall, NULL, pass, 1))

This comment has been minimized.

Copy link
@Headline

Headline Nov 7, 2018

Member

ditto

vptr += sizeof(CBaseEntity *);
*(char **)vptr = szNewTag;

pWrapper->Execute(vstk, NULL);

This comment has been minimized.

Copy link
@Headline

Headline Nov 7, 2018

Member

ditto

@sneak-it

This comment has been minimized.

Copy link

sneak-it commented Nov 7, 2018

No more crashes with latest commit (windows)! :)

@Drifter321

This comment has been minimized.

Copy link
Member Author

Drifter321 commented Nov 7, 2018

As mentioned on IRC, I'm going to ignore the nits, the main goal of this pr is to actually fix and is currently inline with the remainder of the extension. A separate PR should be done fixing all of them at once after this gets merged.

Copy link
Member

asherkin left a comment

What's the plan for 1.9? Are we taking this immediately there or fixing the gamedata only for now?

@Drifter321

This comment has been minimized.

Copy link
Member Author

Drifter321 commented Nov 7, 2018

@asherkin I have no preference. I have the sig already fixed locally if we want to keep 1.9 the same.

@Headline

This comment has been minimized.

Copy link
Member

Headline commented Nov 7, 2018

@asherkin Is it possible that we can take these changes into both, but also update that gamedata signature so people don’t have to update entire sm installations for this signature change in particular?

}

char *pVar = GetPlayerVarAddressOrError<char>(varName, pContext, pPlayer);
if (pVar)

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Nov 7, 2018

Member

style would be !pVar here but this is okay too.

g_pSM->Format(szSizeName, sizeof(szSizeName), "%sSize", varName);

int maxlen = 0;
if(!g_pGameConf->GetOffset(szSizeName, &maxlen))

This comment has been minimized.

Copy link
@KyleSanderson
@KyleSanderson

This comment has been minimized.

Copy link
Member

KyleSanderson commented Nov 7, 2018

I'm not too worked up about any of this (I don't like the style of the existing code now that was shuffled; but that's okay).

My preference would be for 1.9 to preserve the gamedata method and 1.10 we move onwards and forwards with this. If it breaks irrevocably we'll backport or pin 1.10.

@Drifter321

This comment has been minimized.

Copy link
Member Author

Drifter321 commented Nov 7, 2018

Updated the gamedata in 1.9 this time it will hopefully not break as easily. Instead of matching the offset of the member to make it unique, it now matches the max length of the member. So this will now be 1.10/central only.

Credit to 🦆 for the idea.

@sneak-it

This comment has been minimized.

Copy link

sneak-it commented Nov 9, 2018

Could 1.10 also get the updated gamedata committed and/or pushed through the updater since this PR hasn't been merged yet?

@Drifter321

This comment has been minimized.

Copy link
Member Author

Drifter321 commented Nov 9, 2018

Could 1.10 also get the updated gamedata committed and/or pushed through the updater since this PR hasn't been merged yet?

I rather just merge this in. I think we are good to do so, @psychonic any objections?

Copy link
Member

psychonic left a comment

Looks good to me.

@Headline I think there's some benefit to keeping consistency with existing code surrounding the rest, at least enough that it doesn't need to be a priority to change. We could still do that in large passes though if we deem necessary. If this were a new extension or a total revamp of one, it'd be different.

@psychonic psychonic merged commit d79c5e0 into master Nov 9, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Headline Headline deleted the cstrike-clantag-change branch Nov 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.