The International Simutrans Forum

Development => Patches & Projects => Incorporated Patches and Solved Bug Reports => Topic started by: ceeac on July 10, 2020, 08:02:41 AM

Title: Fix for potential crash when splitting or merging paks
Post by: ceeac on July 10, 2020, 08:02:41 AM
When splitting or merging paks, makeobj may crash due to a use-after-scope bug. This patch fixes the bug.
Title: Re: Fix for potential crash when splitting or merging paks
Post by: Dwachs on August 29, 2020, 01:19:52 PM
What is the point in deleting "using std::string" ? It makes the patch hard to read.
Title: Re: Fix for potential crash when splitting or merging paks
Post by: ceeac on August 30, 2020, 06:01:38 PM
IMO the code is easier to read with "using std::string" removed because having a std:: prefix  provides the information that whatever comes after is part of the Standard Library. But I do not have a strong opinion on this. I attached an updated patch without the "using std::string" changes.
Title: Re: Fix for potential crash when splitting or merging paks
Post by: Dwachs on August 31, 2020, 06:28:31 AM
I do not see the crash from the patch. Which variable is used after scope? I see that name is changed inside these methods but it is not used afterwards. Still confused.
Title: Re: Fix for potential crash when splitting or merging paks
Post by: ceeac on September 02, 2020, 06:17:31 PM
In root_writer_t::copy, line 290 in current trunk there is this piece of code:
Code: [Select]
if (outfp == NULL) {
name = find.complete(name, "pak").c_str();
outfp = fopen(name, "wb");
}
find.complete returns by value, and a pointer to the temporary value gets assigned to name, and name gets used afterwards. The same is true for root_writer_t::uncopy (line 361).
Title: Re: Fix for potential crash when splitting or merging paks
Post by: prissi on September 02, 2020, 11:30:09 PM
The code is weird anyway. Why first delete the content of an existing file by open it for writing, then closing it, delete it and to write a temporary file only to finally rename it to the name of the file it deleted before. That logic is broken indeed.

Since the file does not need to be closed after opening and removing all existing content, the use of name after it could have been fixed with
Code: [Select]
- name = find.complete(name, "pak").c_str();
- outfp = fopen(name, "wb");
+ outfp = fopen(find.complete(name, "pak").c_str(), "wb");
see diff
Title: Re: Fix for potential crash when splitting or merging paks
Post by: Dwachs on September 03, 2020, 06:19:26 AM
@ceeac thanks for the explanation (and spotting this)