Depo crashed server

[Xalamander777]

Priority

High

Area

• [ ] Datapack
• [ ] Source
• [ ] Map
• [ ] Other

What happened?

This error crashed server and duplicated item from depo box to inbox container

What OS are you seeing the problem on?

Windows

Code of Conduct

• [X] I agree to follow this project's Code of Conduct

[dudantas]

To better address the issue, it would be helpful to provide more specific information about the problem you are experiencing. Without knowing the steps to reproduce the crash, it is difficult to test and fix the issue. Programming is complex, and there are numerous factors that can contribute to crashes, so having additional details is essential to identify and solve the problem. While crash logs can be useful for diagnosing simple crashes, they are often insufficient for more complex issues. Therefore, please provide as much information as possible to help us resolve the problem quickly and effectively.

[lucascebertin]

I had a consistent scenario here, just found out that some letters written in previous versions (dont know exactly, at least 5 months ago) can cause this behavior. If you guys create a letter with god /i 3505, move it to your first depot box, search it like select * from player_depotitems and update the "attributes" field to my previous content, you could try to read it and crash the server.

update player_depotitems set attributes = unhex('0696002320546865204963652049736C616E64732051756573740A0A2D2035204261742057696E67730A2D2034204265617220506177730A2D203320426F6E656C6F726420457965730A2D203220466973682046696E730A2D203120477265656E20447261676F6E205363616C650A0A5465726D696E61722061206D697373E36F20636F6D205369666C696E6420287376617267726F6E6429120C026A62130E00434841522054455354') where itemtype = 3505 and player_id = YOUR_PLAYER_ID_HERE;

This hex stuff is just for preserve the binary data for the attributes column, the ASCII content is:

–# The Ice Islands Quest

- 5 Bat Wings
- 4 Bear Paws
- 3 Bonelord Eyes
- 2 Fish Fins
- 1 Green Dragon Scale

Terminar a missão com Siflind (svargrond)jbCHAR TEST

The exception and the crash:

terminate called after throwing an instance of 'fmt::v9::format_error'
  what():  time_t value out of range
 Aborted (core dumped)

[Tefcio]

I have the same problem, still crashing. Crashes temporary stopped after i have deleted all leters from database.

[dudantas]

I have the same problem, still crashing. Crashes temporary stopped after i have deleted all leters from database.

try this

void ProtocolGame::sendTextWindow(uint32_t windowTextId, Item* item, uint16_t maxlen, bool canWrite) {
    NetworkMessage msg;
    msg.addByte(0x96);
    msg.add<uint32_t>(windowTextId);
    AddItem(msg, item);

    if (canWrite) {
        msg.add<uint16_t>(maxlen);
        msg.addString(item->getAttribute<std::string>(ItemAttribute_t::TEXT));
    } else {
        const std::string &text = item->getAttribute<std::string>(ItemAttribute_t::TEXT);
        msg.add<uint16_t>(text.size());
        msg.addString(text);
    }

    const std::string &writer = item->getAttribute<std::string>(ItemAttribute_t::WRITER);
    if (!writer.empty()) {
        msg.addString(writer);
    } else {
        msg.add<uint16_t>(0x00);
    }

    msg.addByte(0x00); // Show (Traded)

    auto writtenDate = item->getAttribute<time_t>(ItemAttribute_t::DATE);
    if (writtenDate != 0 && writtenDate < std::numeric_limits<time_t>::max()) {
        msg.addString(formatDateShort(writtenDate));
    } else {
        msg.add<uint16_t>(0x00);
    }

    writeToOutputBuffer(msg);
}

[dudantas]

I had a consistent scenario here, just found out that some letters written in previous versions (dont know exactly, at least 5 months ago) can cause this behavior. If you guys create a letter with god /i 3505, move it to your first depot box, search it like select * from player_depotitems and update the "attributes" field to my previous content, you could try to read it and crash the server.

update player_depotitems set attributes = unhex('0696002320546865204963652049736C616E64732051756573740A0A2D2035204261742057696E67730A2D2034204265617220506177730A2D203320426F6E656C6F726420457965730A2D203220466973682046696E730A2D203120477265656E20447261676F6E205363616C650A0A5465726D696E61722061206D697373E36F20636F6D205369666C696E6420287376617267726F6E6429120C026A62130E00434841522054455354') where itemtype = 3505 and player_id = YOUR_PLAYER_ID_HERE;

This hex stuff is just for preserve the binary data for the attributes column, the ASCII content is:

��# The Ice Islands Quest

- 5 Bat Wings
- 4 Bear Paws
- 3 Bonelord Eyes
- 2 Fish Fins
- 1 Green Dragon Scale

Terminar a missão com Siflind (svargrond)��jb��CHAR TEST

The exception and the crash:

terminate called after throwing an instance of 'fmt::v9::format_error'
  what():  time_t value out of range
 Aborted (core dumped)

Makes sense, you are probably losing information (and exceeding the maximum allowed value of time_t) when converting the old letter to the new attribute.

Try the fix I suggested above. If it does, I'll make a more robust fix.

[lucascebertin]

I tried that code but still crashing, simply adding a try/catch block around this

try {
  auto writtenDate = item->getAttribute<time_t>(ItemAttribute_t::DATE);
  SPDLOG_ERROR("date: {}", writtenDate);
  if (writtenDate != 0 && writtenDate < std::numeric_limits<time_t>::max()) {
    msg.addString(formatDateShort(writtenDate));
  } else {
    msg.add<uint16_t>(0x00);
  }
} catch(const std::exception& e) {
  SPDLOG_ERROR("Error reading time_t attribute from item: {}", e.what());
  msg.add<uint16_t>(0x00);
}

and inside catch block adding msg.add<uint16_t>(0x00); makes the letters, blackboard and labels open but without the time being shown.

I know that adding a try/catch is not a fix, just bypass the error but your assumptions are right, it's the correct location to debug.

[lucascebertin]

I forgot to mention the values received in writtenDate and std::numeric_limits<time_t>::max()

writtenDate: 6196968563679748558
max allowed: 9223372036854775807

Max allowed is bigger than writtenDate and in this case the formatDateShort was called.

[lucascebertin]

Looking deeper into this, looks like timestamp format consider only 32bit size and because of it support is limited to the date January 19, 2038. Another thing is, fmtlib doesn't like big numbers as you can see here.

Other ifpossibilities below:

if (writtenDate != 0 && writtenDate < std::numeric_limits<int32_t>::max())

or

if (writtenDate != 0 && writtenDate < 253402311599) // this number represents 31/12/9999 23:59:59 as (dd/MM/yyyy hh:mm:ss)

Sorry for this mass comment flood.

[dudantas]

I tried that code but still crashing, simply adding a try/catch block around this

try {
  auto writtenDate = item->getAttribute<time_t>(ItemAttribute_t::DATE);
  SPDLOG_ERROR("date: {}", writtenDate);
  if (writtenDate != 0 && writtenDate < std::numeric_limits<time_t>::max()) {
    msg.addString(formatDateShort(writtenDate));
  } else {
    msg.add<uint16_t>(0x00);
  }
} catch(const std::exception& e) {
  SPDLOG_ERROR("Error reading time_t attribute from item: {}", e.what());
  msg.add<uint16_t>(0x00);
}

and inside catch block adding msg.add<uint16_t>(0x00); makes the letters, blackboard and labels open but without the time being shown.

I know that adding a try/catch is not a fix, just bypass the error but your assumptions are right, it's the correct location to debug.

in this case, I think the try/catch needs to be inside the formatDateShort function, as this can happen in other places as well.

[lucascebertin]

In order to prevent even more crashes from happening, I agree. This try/catch + some SPDLOG_ERROR should be enough to give some clues about what is happening.

Something like this?

std::string formatDateShort(time_t time) {
  try {
    return fmt::format("{:%Y-%m-%d %X}", fmt::localtime(time));
  } catch (const std::exception& e) {
      SPDLOG_ERROR("formatDateShort error: {}", e.what());
      return "";
  }
}

And usage?

auto writtenDate = item->getAttribute<time_t>(ItemAttribute_t::DATE);
if (writtenDate != 0 && writtenDate < std::numeric_limits<int32_t>::max()) { // is it still valid to check as int32?
  msg.addString(formatDateShort(writtenDate));
} else {
  msg.add<uint16_t>(0x00);
}

[dudantas]

std::string formatDateShort(time_t time) {
  try {
    return fmt::format("{:%Y-%m-%d %X}", fmt::localtime(time));
  } catch (const std::exception& e) {
      SPDLOG_ERROR("formatDateShort error: {}", e.what());
      return "";
  }
}

Something like this (should return an empty value even outside of ifs):

std::string formatDateShort(time_t time) {
  std::string date;
  try {
    date = fmt::format("{:%Y-%m-%d %X}", fmt::localtime(time));
  } catch (const std::exception& e) {
      SPDLOG_ERROR("formatDateShort error: {}", e.what());
  }
  return date;
}

Also note that we don't need to add "" to the string, by default it is already empty. The call would be normal, it doesn't need numeric limits anymore The only question I think is if catch should return a default value, for example the maximum allowed

[dudantas]

Looking deeper into this, looks like timestamp format consider only 32bit size and because of it support is limited to the date January 19, 2038. Another thing is, fmtlib doesn't like big numbers as you can see here.

Other ifpossibilities below:

if (writtenDate != 0 && writtenDate < std::numeric_limits<int32_t>::max())

or

if (writtenDate != 0 && writtenDate < 253402311599) // this number represents 31/12/9999 23:59:59 as (dd/MM/yyyy hh:mm:ss)

Sorry for this mass comment flood.

So the problem can't be that you're running a 32-bit system?

[lucascebertin]

Looking deeper into this, looks like timestamp format consider only 32bit size and because of it support is limited to the date January 19, 2038. Another thing is, fmtlib doesn't like big numbers as you can see here. Other ifpossibilities below:

if (writtenDate != 0 && writtenDate < std::numeric_limits<int32_t>::max())

or

if (writtenDate != 0 && writtenDate < 253402311599) // this number represents 31/12/9999 23:59:59 as (dd/MM/yyyy hh:mm:ss)

Sorry for this mass comment flood.

So the problem can't be that you're running a 32-bit system?

No, I'm not running it on a 32bit system, I mean the concept of timestamp is based on a 32bit max value.

from https://www.unixtimestamp.com/ "What happens on January 19, 2038? On this date the Unix Time Stamp will cease to work due to a 32-bit overflow. Before this moment millions of applications will need to either adopt a new convention for time stamps or be migrated to 64-bit systems which will buy the time stamp a "bit" more time."

About returning a max value as default, personally I prefer something empty. In other places that is used the default value could be misleading and "looks like" real for the users (like banned accounts where that function is being called).

[dudantas]

lucascebertin

Yes, I agree that returning the empty value seems to make more sense.