search

adventuregamestudio / ags

AGS editor and engine source code
Other
707 stars 159 forks source link

Engine: use safe strcpy function instead of snprintf where truncation expected #2569

Closed ivan-mogilko closed 2 weeks ago

ivan-mogilko commented 2 weeks ago

Fix #2567

This replaces uses of snprintf in the code where truncation of the source string is intended. Added a "ags_strncpy_s", using "strncpy_s" from the C11 extension as a reference (see https://en.cppreference.com/w/c/string/byte/strncpy)

ivan-mogilko commented 2 weeks ago

@ericoporto i copied error values and some parts from your ags_strncpy_s implementation in ags4, but copy characters in a for loop in place instead of calling snprintf, because extra call to a formatting function seems unnecessary in this case.

Also added asserts to help catch errors.

ericoporto commented 2 weeks ago

Looks fine, there is a change to Engine/main/update.cpp I can’t tell if it was intended or not.

ivan-mogilko commented 2 weeks ago

Looks fine, there is a change to Engine/main/update.cpp I can’t tell if it was intended or not.

Oops, that was a fix for another warning, it's already in master.

ericoporto commented 2 weeks ago

This appears to work fine but I found an error when loading a save - I was testing playing mags version of The Shivah. I also tried running it with master and got the same error - created a new save with the master version and then tried to load it. Because it's on master too, it's not related to this PR, so I would say the PR appears to work alright, and would be fine to merge it. I will leave the unrelated error reported below.

image

An internal error has occurred. Please note down the following information.
If the problem persists, contact the game author for support or post these details on the AGS Technical Forum.
(Engine version 3.6.2.2)

Error: Unable to restore the saved game.
Failed to restore the savegame component.
(#12) Script Modules, version 3060200, at offset 56866.
Saved content does not match current game.
Save is missing a script module that exists in the game: Internal built-in header.
ivan-mogilko commented 2 weeks ago

I also tried running it with master and got the same error - created a new save with the master version and then tried to load it.

This may be related to the merged changes from #2489, or earlier changes which save script modules with their names...

ivan-mogilko commented 2 weeks ago

In regards to this PR, I tested building using GCC 9.4.0 (the topmost GCC i can install on Ubuntu 16 LTS), and this fixes 3 warnings with snprintf truncations.

ivan-mogilko commented 2 weeks ago

@ericoporto I pushed 2 fixes to the master, this problem was introduced at an earlier change to save format: 06363d796b02ffee65de45e60073dbff17ecdaa0

But apparently this does not happen in newer compiled games (post 2.x ?), due to the difference in how script data is filled.

ericoporto commented 2 weeks ago

I confirm that the fix on master indeed works! Can save and load fine when playing mags version of The Shivah.