News:

Do you need help?
Simutrans Wiki Manual can help you to play and extend Simutrans. In 9 languages.

Fix for potential crash when splitting or merging paks

Started by ceeac, July 10, 2020, 08:02:41 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

ceeac

When splitting or merging paks, makeobj may crash due to a use-after-scope bug. This patch fixes the bug.

Dwachs

What is the point in deleting "using std::string" ? It makes the patch hard to read.
Parsley, sage, rosemary, and maggikraut.

ceeac

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.

Dwachs

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.
Parsley, sage, rosemary, and maggikraut.

ceeac

In root_writer_t::copy, line 290 in current trunk there is this piece of code:

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).

prissi

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

- name = find.complete(name, "pak").c_str();
- outfp = fopen(name, "wb");
+ outfp = fopen(find.complete(name, "pak").c_str(), "wb");

see diff

Dwachs

@ceeac thanks for the explanation (and spotting this)
Parsley, sage, rosemary, and maggikraut.