Fix multi-account password saving

[vgaming]

Fixes GH-6288, and hopefully makes the code
more readable / easier to understand.

[vgaming]

I tested the new code locally, it works both for single-account use case and for storing/loading multiple accounts

[vgaming]

I also hate to admit, but I cannot pinpoint where exactly the old code was broken. In my perception that was just messy code that is really hard to understand, with unneeded additional variable offset, triple resizes that adjust for escaped and not escaped keys... A lot of unnecessary complexity. In the issue GH-6288 itself, however, it was observed that credentials file is not split correctly, with the CREDENTIAL_SEPARATOR obviously broken. I can copy more of the relevant IRC discussions here if there's a question

[Wedge009]

As discussed in #wesnoth-dev, it is indeed strange that it didn't expose the issue for multi-account credential saving for you in 1.14. But it does look like replacing manual std::copy and offset tracking is a good thing if it solves the bug for you and @Hejnewar.

It looks like only @CelticMinstrel has done any meaningful work on this code, so I'll suggest he review this before merging.

[vgaming]

UPD: When using this branch on an old corrupted credentials file from wesnoth-1.16.1, the passwords have still to be entered once, but they're saved successfully and for the user it works as expected. (This PR doesn't really change reading of the credentials file.) What remains however is that the credentials file will keep those extra broken/unneeded entries, even with the new PR. They'll just stay as garbage around, without use.

[vgaming]

To provide a bit of context for people who weren't today in IRC. Without this PR, for single-account credentials file, the entry in credentials file looks like: server: server.wesnoth.org:15000, login: vasili. For multi-account credentials file, without the PR, corrupted entries will be created upon save, such as: server: server.wesnoth.org:15000, login: login: vasili@server.wesnoth.org:15000=�����;sa��vasya2 (the login spans much further than it should, including the server and probably password of the previous/next entries)

[CelticMinstrel]

Looks good, just one minor change to make.

[CelticMinstrel]

(0, CREDENTIAL_SEPARATOR) does nothing - just remove it.

[vgaming]

Thanks for the review! Addressed the comment

[AI0867]

The only potential bug I see in the old code is that the second resize (for the difference in size between escaped and non-escaped keys) doesn't pad the buffer with CREDENTIAL_SEPARATOR.

The new code looks like a definite improvement. It might be a bit slower due to not pre-allocating the buffer, but this is hardly a performance-critical path.

[vgaming]

@AI0867 totally agree. Also, for this code path, I'd indeed prefer readability, maintainability and less bugs, over performance optimization %insert-donald-knuth-quote-here%

[AI0867]

Mostly unrelated, but I'm unsure if I should open a new issue for this: I noticed a TODO to do key stretching on the autogenerated key. This seems like a good idea, but unfortunately, it comes with a whole host of backwards-compatibility issues, especially if we want to be able to allow older versions of wesnoth to keep using the same credentials file.

Should we open a new issue for that to think about what a good design would even look like?

[vgaming]

@AI0867 I don't know, I can't really say. Normally I always try to prioritize fixing bugs, and only then doing new features. But that is subjective of course.