pvpgn crash build #74b06d0

[Kellsya]

Downloaded latest build from https://pvpgn.pro and compiled but ready compiled version will crash the same way. Version: pvpgn-master-1.99.7.1.1-PRO-74b06d0 Crash occurs right when connecting to pvpgn with diablo loader Crash dmp here: https://www.sendspace.com/file/jiu1o1

The crash isn't present in old version like 1.99.7.1-PRO released on Sep 11, 2016

[Kellsya]

@RElesgoe, maybe something to do with versioncheck I assume?

[RElesgoe]

Can you post a crash dump from one of the debug builds and tell me which one you used (e.g. pvpgn-master-1.99.7.1.1-PRO-74b06d0_plain_debug.zip). Also, post logs.

[Kellsya]

I already did, Crash dmp here: https://www.sendspace.com/file/jiu1o1 That's the crash dmp I get in debug mode I've used pvpgn-server-master.tar.gz (source code) compiled in debug mode as well as pvpgn-master-1.99.7.1.1-PRO-74b06d0_plain.zip

[RElesgoe]

I need a crash dump that is created when using one of PvPGN's pre-compiled debug builds. Use the PvPGN from pvpgn-master-1.99.7.1.1-PRO-74b06d0_plain_debug.zip (not pvpgn-master-1.99.7.1.1-PRO-74b06d0_plain.zip) and then upload the dump file produced by that PvPGN.

[Kellsya]

Here it is https://www.sendspace.com/file/uebxa9

[RElesgoe]

Call Stack

>   PvPGN.exe!std::basic_string<char,std::char_traits<char>,std::allocator<char> >::basic_string<char,std::char_traits<char>,std::allocator<char> >()
    PvPGN.exe!pvpgn::bnetd::VersionCheck::get_version_tag()
    PvPGN.exe!pvpgn::bnetd::handle_apireg_packet()
    PvPGN.exe!pvpgn::bnetd::client_init_email()
    PvPGN.exe!pvpgn::bnetd::handle_bnet_packet()
    PvPGN.exe!pvpgn::eventlog<char const *,int,char const *>()
    PvPGN.exe!pvpgn::eventlog<char const *,int,char const *>()
    PvPGN.exe!pvpgn::FDWSelectBackend::del()
    PvPGN.exe!pvpgn::fdwatch_traverse()
    PvPGN.exe!pvpgn::FDWSelectBackend::handle()

[RElesgoe]

I found the error, compile from the develop branch and then let me know if it works for you. It will take a while for the binaries to appear on pvpgn.pro.

[Kellsya]

@RElesgoe, thank you it's fixed now. Can you please push the fix to stable branch.

Also small questions if you can please answer:

1. In terms of stability is recommended to use the latest stable version 1.99.7.1.1 or the older 1.99.7.1

2. After compiling latest branch I got an error in pvpgn log while using mysql: I also got this error first time I compiled 1.99.7x with mysql support but restoring database again fixed it.

   [error] pvpgn::bnetd::accountlist_add_account: got bad account (empty username) [error] pvpgn::bnetd::_cb_read_accounts: could not add account to list

I deleted my mysql pvpgn database and restored then started pvpgn again and it works fine. Any idea what happend and how can this be avoided? It's a 1.8.5 mysql database

3. What is the easiest way to move from 1.8.5 to 1.99.7.x in terms of editing database? 1.8.5 has a database named auth_lockk, while 1.99.7.x has auth_lock. Should I rename the old table auth_lockk to auth_lock in order to keep all current account locks? Because when 1.99.7 creates the new missing tables in 1.8.5 database all active locks are ignored and banned players can login fine. Could you tell me what's the best way to transfer bans to the new database?

4. Any other changes in terms of database that I need to make for my old 1.8.5 database? The few missing tables will be automatically added but for example there are some tables like account pass hash that are 40 size in 1.99.7 and 128 in 1.8.5. All hash passwords are 40 characters long, table size is usually 128 for most stables in 1.8.5 so it's ok if I leave the tables of 1.8.5 unmodified except auth_lokk that I asked about in point 3?

Thank you for your time.

[RElesgoe]

1. The latest commit in the develop branch should be stable enough, but like all other releases, there has been little testing. I'm about to merge the develop branch into master and release 1.99.7.2.0 (#347) because I think it's good enough.

For all of your other questions, I don't know the answer. I'm guessing @HarpyWar would know.

[Kellsya]

@RElesgoe, it will be merged later today? Does running the whole pvpgn in debug mode but with alerts set only for fatal error would be ok to do, won't causes overload, leaks or other issues? Just in case it crashes so it generates a dump file if being in debug mode. I see some servers with more players use 1.99.7.1 so I wonder if it's better and more stable to use that instead 1.99.7.1.1 or 1.99.7.2.0. What would you think?

@HarpyWar, mind answering me to 2, 3 and 4?

Thank you.

[HarpyWar]

2. accountlist_add_account function just reads BNET\acct\username from a database and alert with "got bad account (empty username)" if it empty. So try find and delete accounts with empty usernames in your database.

3. AFAIK this is only a single correction auth_lockk -> auth_lock. The structure of database the same, new several fields will be created automatically on demand.

4. I restricted size of fields with known size. For example, size of passhash string can not be > 40 bytes, but it was 128. You can compare conf/sql_DB_layout.conf from both pvpgn versions and take new sizes from there, if you want to optimize size of your database a little.

[Kellsya]

2. Is it safe to simply delete the row or I should better add some text data manually instead like some account that no one will ever create. Indeed, in my database record 0 (the very first line) has empty username password.

3. Great, thank you.

4. Yes, that's what i did. Wanted to make sure resizing to what it should be normally the max size will not cause problems.

[RElesgoe]

@Kelly32 I wasn't able to utilize your dump for some reason, so I wouldn't rely on dumps. I would compile PvPGN in Release mode and as a console (i.e. without the GUI) and enable all logging levels.

[Kellsya]

So 1.99.7.1.1 or the older 199.7.1 then?

[RElesgoe]

The latest commit in the develop branch should work fine. I know the GUI will slow down the server, but I'm not sure about performance when the server has a lot of concurrent users. If you find that performance is subpar on your server, open an issue and I might look into it.

[Kellsya]

Can you push the crash fix for unknown version to master branch and make a stable release with source available to download from https://pvpgn.pro/? Thank you

[RElesgoe]

It's merged

[HarpyWar]

The latest commit in the develop branch should work fine. I know the GUI will slow down the server, but I'm not sure about performance when the server has a lot of concurrent users. If you find that performance is subpar on your server, open an issue and I might look into it.

Dump files did not contain debug information, because of that this dump was useless. I fixed it in https://github.com/pvpgn/pvpgn-server/commit/4d1025dd9dde8fa204d82a9e5916c5568b75970e by adding a flag /Zi.

Also I added crash dump generation for Release, so I think Debug builds should not be generated. Why to add crash dump to Release is explained here https://www.wintellect.com/correctly-creating-native-c-release-build-pdbs Pdb files do not affect on performance https://www.wintellect.com/do-pdb-files-affect-performance/

[Kellsya]

So if I download latest release version from https://pvpgn.pro/ now and then compile as release it will generate a crash dump file in case of crash?

[HarpyWar]

Not now, because the commit was pushed into develop branch. But I think it could be moved into master. There are only builds from master on https://pvpgn.pro. You can use Magic Builder to create own build from any branch.

[Kellsya]

Do I need to update magic builder too after this new feature including dump for release is added to release on website?

And one question, is safe to submit dump files to public here and no sensitive or personal data is revealed yes? like database password or any password on server where pvpgn is hosted?

[RElesgoe]

@harpywar instead of building Release and Debug configurations, just use the RelWithDebInfo configuration which CMake automatically provides. There wouldn't be a need to have separate Release and Debug builds if you use this.

@kelly32 the file path to PvPGN will be visible, so if you have your name in the file path and are concerned about that, move PvPGN to another directory.

[HarpyWar]

You dont need to update Magic Builder in this case. Furthermore it can be updated automatically if a new version released (not yet).

A crash should dump contain only data from a local scope of the function where a crash has been occured. At least I could not get storage_path from an artificial created dump. But it may be possible if a crash will be occured on a program start in a specific function which reads this configuration value.

[HarpyWar]

@RElesgoe it's required to modify CMakeLists.txt somehow to restore RelWithDebInfo, or we can keep current Release settings which I commited?

[Kellsya]

Ok, thank you .Crash dumps will be generated for all executable files or only pvpgn?

[HarpyWar]

@Kelly32 d2cs and d2dbs also have this feature now.

@RElesgoe I'm going to push latest commits from develop into master, it will generate pdb files for Release configuration, as I wrote above. (Looks like RelWithDebInfo does not optimize binaries with flags /OPT:REF /OPT:ICF, which are describe on the article, which is also mentioned above.. And Magic Builder already support Release configuration, but not RelWithDebInfo.)

Also modify AppVeyor, which will generate two archives: one with Release binaries, and the second with pdb files from these binaries (it will allow to download pdb in any time when it's required to debug crash dumps of the related commit, and thus decrease size of the main archive of binaries).

Let me know if you have additional thoughts.

[Kellsya]

@HarpyWar. that's great because it will be easier to find out reason of crash now! Please let me know when this feature is pushed to last release source available on https://pvpgn.pro/ so I can download it.

[HarpyWar]

The changes merged into master. Also I updated binaries in Releases section of the repo.

[Kellsya]

Thank you very much, will download it right away and compile.

[RElesgoe]

@HarpyWar It's incorrect to update the binaries for 1.99.7.2.0 in the Releases section. The binaries should reflect the commit at the time of release.

It's more appropriate to update the version number to 1.99.7.2.1.

[HarpyWar]

@RElesgoe we can delete the current release and create a new with the same contents to keep the commit id equal to the left side. Basically your proposal is better, but in this case this one can be also acceptable.

[RElesgoe]

@HarpyWar I don't think it's acceptable because each release should have a unique version. If someone downloads the binaries before the new binaries are uploaded, then there are two conflicting binaries whose version is 1.99.7.2.0. Once released, it should be the way it is.

[HarpyWar]

We will stick to this way for releases in the future. That was an exclusion. BTW commit hash is appended to a version string, so the difference is still there.

[Kellsya]

Hello. The following error is present in pvpgn when I start it.

[error] pvpgn::bnetd::load_versioncheck_conf: Could not open file "conf\versioncheck.conf" for reading [error] pre_server_startup: could not load versioncheck list

I think because versioncheck file has been changed to json. How can the error be removed? I made a copy of versioncheck.json and renamed it to versioncheck.conf and the error is gone but is this the right way to do it?

And one more thing, new version doesn't seem to create LADDER files inside /var/ladders on a clean install and I didn't had this issue with previous pvpgn version. I copied them from my previous installation and it's working now.

[RElesgoe]

Did you use a fresh bnetd.conf from installation or did you copy an old one over? Check this line in your bnetd.conf: https://github.com/pvpgn/pvpgn-server/blob/master/conf/bnetd.conf.in#L85

[Kellsya]

Indeed, was using the previous bnetd.conf file as I had the setting saved and assumed it was the same as in the previous revision since there was the same .json file. That would do, thank you.

[Kellsya]

One more thing if you or @HarpyWar can help. I see the clienttag and user status part was changed in command.cpp and problem is when I try to display users it display total users on server only. I want to have it like old pvpgn versions before pro where it displays in one command both total users on pvpgn, games, channels and below that line total users playing only warcraft3 or diablo game depending what the user is playing at that momend and I noticed that diablo tag is now called DIABLOSHR instead of DIABLO2XP. How can I replace the _handle_stats_command and handle_status_command functions with the old ones https://pastebin.com/mjwCXtEy and https://pastebin.com/4KtUNPBs or any way to separate statistics and make them show total and per game when using a single command like in pvpgn 1.8.5 and 1.99 ?

[HarpyWar]

I don't remember such output of this command by default. But may be this part was changed by previous developers. You can use /status D2XP to display users and games of the specified game client.

[HarpyWar]

I just wrote Lua function which overrides the /status command with the output you wrote above. This is also a nice example how to utilize Lua script. To use put the file into lua/command/status.lua and add a command definition inside lua/handle_command.lua:

local lua_command_table = {
    [1] = {
...
        ["/status"] = command_status, ["/users"] = command_status,
...

Then /rehash lua to reload scripts. status.zip

[Kellsya]

@HarpyWar, here is the original file from pvpgn. When you use /status or /users command in all pvpgn versions before this branch -pro it will list total users and individual users at the same time so you don't need to use in a separate command /status w3xp to show you warcraft players. https://sourceforge.net/projects/pvpgn.berlios/files/

Thanks for the lua function, but is it safe to enable lua support? I read that lua is still experimental and I'd rather have it disabled since I don't use any of it's functions or need to disable what I don't use in case I forget something theere.

[HarpyWar]

Lua is safe to use on small and medium servers. On large servers it may cause performance issues. But it also depends on used Lua scripts. So, until CPU is not overloaded, Lua can be enabled. I would prefer to use it instead of not to use.

[Kellsya]

Any way the function can be replaced with the original one in this case?

[HarpyWar]

Sure it can, but I can't tell you exactly how to do it. You should look how it's implemented in old and new versions and find differences. I wanted to say that enabled Lua does not affect on performance. Only used Lua scripts, mostly boxing of objects in iterations because of bad integration. The raw Lua is very fast.

[Kellsya]

Do I need to delete all files or disable in config.lua not necessary settings like quiz = true, starcraft ah and i see ghost is already disable so not working?

[HarpyWar]

It's enough set to false options in config.lua to disable unused functionality. And remove unwanted command definitions from handle_command.lua.

[Kellsya]

I get this error in pvpgn when I try to use the command. [error] pvpgn::bnetd::lua_handle_command: lua/extend/account_wrap.lua:143: attempt to index global 'api' (a nil value)

[HarpyWar]

I can't reproduce the error with the latest pvpgn. Probably you edit other Lua scripts somehow or your pvpgn source was modified? api is a global object with an access pvpgn features from Lua https://github.com/pvpgn/pvpgn-server/blob/master/src/bnetd/luainterface.cpp#L123-L157

Here is default lua directory with new /status command lua.zip

[Kellsya]

@HarpyWar, maybe I did the changes in the wrong source before so I made fresh recompile and works perfect now, thanks so much.