Truncation warning in Common/game/main_game_file.cpp

[tag2015]

Describe the bug

In line 651 in UpgradeCharacters: warning: ‘snprintf’ output may be truncated before the last format character [-Wformat-truncation=] snprintf(chars[i].scrname, LEGACY_MAX_SCRIPT_NAME_LEN, "c%s", namelwr);

This is quite obvious since it adds a "c" to the string and both source and dest are the same size (20).

[ericoporto]

Just adding a link to make it easier to find it

https://github.com/adventuregamestudio/ags/blob/f8f47f233fbddf622d62850bb6d1fc08139c1b98/Common/game/main_game_file.cpp#L651

It looks like just using LEGACY_MAX_SCRIPT_NAME_LEN-1 would be enough here.

[ivan-mogilko]

@tag2015 how exactly can this be fixed, if the idea is to truncate it, one way or another?

If I reduce the previous buffer size by -1, then there still will be truncation, only one step earlier.

• take 20-chars long original buffer
• truncate to 19-chars long temp buffer, do lowercase over it
• copy result to 20-chars long buffer.

Right now the first copy is done by memcpy, but then it could as well be done using snprintf too, for safety (guarantee null terminator). Like this code:

        char namelwr[LEGACY_MAX_SCRIPT_NAME_LEN - 1];
        for (int i = 0; i < numcharacters; i++)
        {
            if (chars[i].scrname[0] == 0)
                continue;
            snprintf(namelwr, sizeof(namelwr), "%s", chars[i].scrname); // <--- truncation from 20 to 19 chars
            ags_strlwr(namelwr + 1); // lowercase starting with the second char
            snprintf(chars[i].scrname, sizeof(chars[i].scrname), "c%s", namelwr);
            chars2[i].scrname_new = chars[i].scrname;
        }

would not that lead to another warning?

---

For the reference on script name length, I made an investigation, and AGS 2.x actually limited character script names to 14 chars, although saved them as 20-chars. Therefore logically there should not be any problems in truncating to 19 chars.

[tag2015]

I'll clarify the matter. It was reported by a scummvm dev that this code causes the following warning (I don't know the compiler used). Anyway the complete warning is as follows (this is ScummVM's codebase, but the code is identical)

engines/ags/shared/game/main_game_file.cpp:574:84: warning: ‘snprintf’ output may be truncated before the last format character [-Wformat-truncation=]
                         snprintf(chars[i].scrname, LEGACY_MAX_SCRIPT_NAME_LEN, "c%s", namelwr);
                                                                                    ^
engines/ags/shared/game/main_game_file.cpp:574:33: note: ‘snprintf’ output between 2 and 21 bytes into a destination of size 20
                         snprintf(chars[i].scrname, LEGACY_MAX_SCRIPT_NAME_LEN, "c%s", namelwr);
                         ~~^~~~~~~~~~~~~~~~

I assumed that the code simply prepends a 'c' to the character name string; in this case the original string may be 20 bytes and since the destination is 20 too that triggers the warning. Anyway if the character script names is indeed limited to 14 chars in AGS 2.x, maybe just using LEGACY_MAX_SCRIPT_NAME_LEN-1 as size, would be fine to silence the warning.

[ivan-mogilko]

Ok, I installed gcc-7, going to build engine with it, see what it reports for "format-truncation" and how these mistakes may be handled...

[ivan-mogilko]

@tag2015 could you clarify the compiler version, or options used?

I tried gcc 7.5, but it reports this "format-truncation" warning in a different place (sys_events.cpp), but not in the one reported here, and I cannot tell why.

[tag2015]

@tag2015 could you clarify the compiler version, or options used?

I can't reproduce the warning either. I'll ask the original reporter

[ivan-mogilko]

I am leaning to an idea of using another function instead of snprintf where we expect intended truncation and dont exactly need formatting. (in this reported case it could be used in step 1, to copy from 20-size array to 19-size array) The standard ones like strncpy seem to not be entirely safe, there's strncpy_s, but it's not available everywhere, so either we could mimic that or write our own of similar style.

[digitall]

@tag2015 I am the original reporter of this warning being emitted. This is emitted from GCC 13.3.1_p20240614 on amd64

[digitall]

@ivan-mogilko: Did you really mean GCC-7 ?

Despite GCC 7.5 being the latest GCC-7 release, it was released on Nov 14 2019. It is now over 4 years old and GCC-15 is current bleeding edge development with GCC-14 being stable: https://gcc.gnu.org/releases.html

Newer versions of GCC (and compilers generally) tend to be stricter, better at linting and detection of problems and thus emit more warnings. I would suggest trying with GCC 13 latest release if possible.

If you are running on Windows, then https://packages.msys2.org/search?q=gcc support this. If you are running on MacOS, then https://formulae.brew.sh/formula/gcc supports this. If you are running on Linux, then you should be able to find that in your distribution repositories or run/bootstrap a toolchain in /opt.

Worst case, there is always a VM or Docker :)

I am happy to test any proposed patch here to see if it fixes the warning... Just drop me a link to a gist

[ivan-mogilko]

Did you really mean GCC-7 ? Despite GCC 7.5 being the latest GCC-7 release, it was released on Nov 14 2019. It is now over 4 years old and GCC-15 is current bleeding edge development with GCC-14 being stable:

Yes, but I was probably doing a wrong thing. I'm using a pretty old version of Ubuntu installed on a virtual machine, so tried to search the earliest GCC that had this warning implemented, which resulted in GCC 7. But I might try a more recent GCC later.

I am happy to test any proposed patch here to see if it fixes the warning...

I opened a PR #2569

[ericoporto]

@ivan-mogilko I added ags_strncpy in string_compat in the PR https://github.com/adventuregamestudio/ags/pull/2508 when I had to build the ags4 compiler using gcc.

[ivan-mogilko]

@ivan-mogilko I added ags_strncpy in string_compat in the PR #2508 when I had to build the ags4 compiler using gcc.

Oh, I could not remember if i've seen this somewhere or not.

My first variant of PR had ags_strncpy_s, but when searching through the web I found that people say it's not a very much liked extension, so wrote separate function.

Okay, I will remake it back to use ags_strncpy_s, for consistency.

[ivan-mogilko]

@digitall I managed to get reported warnings using gcc-9, that's the topmost gcc that can be installed on Ubuntu 16 LTS which i currently use from an official package.

Also got 2 more similar warnings in other places with this. Amended #2569.

[digitall]

@ivan-mogilko Thanks. I think those changes should fix the warnings across all GCC versions, but will let you know if there is any further issues.