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

Fix string view in game world auth #4360

Closed

Conversation

ArturKnopik
Copy link
Contributor

Fix string_view pointing to non-existent string

Pull Request Prelude

  • I have followed [proper The Forgotten Server code styling][code].
  • I have read and understood the [contribution guidelines][cont] before making this PR.
  • I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Issues addressed:
#4359

@DSpeichert DSpeichert requested a review from ranisalt April 1, 2023 17:25
@ranisalt
Copy link
Member

ranisalt commented Apr 1, 2023

No need for most of these changes; just changing the return type from string_view to string in gameworldAuthentication and moveing it into the lambda is enough.

@ArturKnopik
Copy link
Contributor Author

ArturKnopik commented Apr 1, 2023

i tried it your way but i failed, i don't know why but i got an empty string after escaping gameworldAuthentication method(int onRecvFirstMessage method), before adding the string to the pair the string had a valid value

AKnopik PC added 3 commits April 2, 2023 11:18
@ArturKnopik
Copy link
Contributor Author

updated, issue with @ranisalt approach was std::tie and characterName variable, characterName was string_view not std::string

@reyaleman
Copy link
Contributor

updated, issue with @ranisalt approach was std::tie and characterName variable, characterName was string_view not std::string

I just changed std::tie to std::pair on protocolgame.cpp (L472) and it worked 😕

std::tie(accountId, characterName) = ...
std::pair(accountId, characterName) = ...

@ranisalt
Copy link
Member

ranisalt commented Apr 2, 2023

I would try with

auto [accountId, name] = gameworldAuthentication(...);

then move name into the lambda if necessary.

std::pair(accountId, characterName) = ...

Did it compile or did it work? I think that's creating a new pair with accountId (which is zero) and characterName (the unnormalized name) and then assigning it, but this shouldn't override the previous values, and you'll get the accountId == 0 and the unmodified characterName instead.

@reyaleman
Copy link
Contributor

std::pair(accountId, characterName) = ...

Did it compile or did it work? I think that's creating a new pair with accountId (which is zero) and characterName (the unnormalized name) and then assigning it, but this shouldn't override the previous values, and you'll get the accountId == 0 and the unmodified characterName instead.

you're right... it doesn't override previous values for accountId and characterName.

Ignore my previous comment 👍

@ranisalt
Copy link
Member

ranisalt commented Apr 2, 2023

To be honest, reading the character name would just prevent a hopelessly broken client from not being able to log in, because by default the Tibia client and OTClient send the name exactly as the login server responds.

We can skip that code and return solely the account ID, the code gets much simpler. Along these lines:

	uint32_t accountId = IOLoginData::gameworldAuthentication(accountName, password, characterName, token, tokenTime);
	if (accountId == 0) {
		disconnectClient("Account name or password is not correct.");
		return;
	}

	g_dispatcher.addTask([=, thisPtr = getThis(), characterName = std::string{characterName}]() {
		thisPtr->login(characterName, accountId, operatingSystem);
	});

Alternatively, we can instead return the character ID and not the name, and then login takes it, since the very first thing it does is getPlayerByName.

What do you think?

@ArturKnopik
Copy link
Contributor Author

ArturKnopik commented Apr 2, 2023

characterName is string_view, is string_view moveable? I don't think so... but i can be wrong

@ranisalt
Copy link
Member

ranisalt commented Apr 2, 2023

characterName is string_view, string_view is moveable? I don't think so... but i can be wrong

Right, it is not, so keep std::string{characterName} to copy it

@ArturKnopik ArturKnopik deleted the fixStringViewInGameWorldAuth branch April 10, 2023 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants