The International Simutrans Forum

 

Author Topic: Fix for potential crash when splitting or merging paks  (Read 532 times)

0 Members and 1 Guest are viewing this topic.

Offline ceeac

  • Devotee
  • *
  • Posts: 249
Fix for potential crash when splitting or merging paks
« 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.

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4869
  • Languages: EN, DE, AT
Re: Fix for potential crash when splitting or merging paks
« Reply #1 on: August 29, 2020, 01:19:52 PM »
What is the point in deleting "using std::string" ? It makes the patch hard to read.

Offline ceeac

  • Devotee
  • *
  • Posts: 249
Re: Fix for potential crash when splitting or merging paks
« Reply #2 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.

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4869
  • Languages: EN, DE, AT
Re: Fix for potential crash when splitting or merging paks
« Reply #3 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.

Offline ceeac

  • Devotee
  • *
  • Posts: 249
Re: Fix for potential crash when splitting or merging paks
« Reply #4 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).

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10635
  • Languages: De,EN,JP
Re: Fix for potential crash when splitting or merging paks
« Reply #5 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

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4869
  • Languages: EN, DE, AT
Re: Fix for potential crash when splitting or merging paks
« Reply #6 on: September 03, 2020, 06:19:26 AM »
@ceeac thanks for the explanation (and spotting this)