Latest Posts

Topic: NetworkGamingMessages::get_message() throws random strings at format()

tothxa
Avatar
Topic Opener
Joined: 2021-03-24, 11:44
Posts: 437
OS: antix / Debian
Version: some new PR I'm testing
Ranking
Tribe Member
Posted at: 2022-09-10, 08:38

I've noticed many format() error messages in the crash logs in #5548 and #5555. I tried to track them down, and I think they come from NetworkGamingMessages::get_message() trying arguments in turn whether they can be used as format strings with format(). That doesn't look safe at all, does it?

With format() caching format strings, I'm afraid it may be even worse.

edit:

I tried to track down callers, and I don't think it is supposed to receive format strings as arguments. So the strange practice is only there because the message templates in the map ngmessages have different number of arguments. My naive way of refactoring would be to change the map (private to this code) and add the number of arguments for each template string manually. Then instead of trial and error the proper format() call could be just looked up.

Now I have to go, so I'll try it later if it's OK.

Edited: 2022-09-10, 09:26

Top Quote
Nordfriese
Avatar
Joined: 2017-01-17, 17:07
Posts: 1949
OS: Debian Testing
Version: Latest master
Ranking
One Elder of Players
Location: 0x55555d3a34c0
Posted at: 2022-09-10, 09:40

Storing the expected number of format arguments along with the string sounds good to me. Caching shouldn't be a problem, we only cache the unformatted source string and its nodes tree (which is valid even if the string has zero placeholders), not any replacement instantiations.


Top Quote
tothxa
Avatar
Topic Opener
Joined: 2021-03-24, 11:44
Posts: 437
OS: antix / Debian
Version: some new PR I'm testing
Ranking
Tribe Member
Posted at: 2022-09-10, 22:38

Yes, I know. My concern is that normally we would have more or less a fixed set of format strings, but here all kind of argument strings are treated as format strings, each of them being added to the cache. But OK, that probably shouldn't be a big issue.

I tried to review once again in format/tree.h whether throwing these exceptions is safe in the regard of properly cleaning up, but I don't really know how to do it.


Top Quote