mbbsemu / MBBSEmu

The MajorBBS Emulation Project is an Open Source, Cross-Platform emulator for easily running The MajorBBS & Worldgroup Modules
https://www.mbbsemu.com
MIT License
128 stars 14 forks source link

[MUICYBER] Lords of Cyberspace - Crashes during nightly cleanup #143

Closed BlaZZZed1980 closed 3 years ago

BlaZZZed1980 commented 3 years ago

Module Information

Describe the bug Causes MBBSEmu to crash when performing nightly cleanup routines

To Reproduce Steps to reproduce the behavior:

  1. Load MUICYBER module
  2. Wait for nightly cleanup to run

Expected behavior Would like cleanup to run properly, or MBBSEmu to keep running after the error

Log File Excerpt

2020-09-19 03:01:25.2517 Info MBBSEmu.HostProcess.MbbsHost.<DoNightlyCleanup>b__44_1 Calling nightly cleanup routine on module MUICYBER
AUDIT SUMMARY: LoC: Clean Up Started 
AUDIT DETAIL: LoC: Clean Up Started 
AUDIT SUMMARY: LoC: Clearing Inactive Chars 
AUDIT DETAIL: LoC: Clearing Inactive Chars 
2020-09-19 03:01:25.2818 Warn MBBSEmu.HostProcess.ExportedModules.Majorbbs.f_read Attempting to read EOF file, returning null pointer
AUDIT SUMMARY: LoC: Checking Tournament 
AUDIT DETAIL: LoC: Checking Tournament 
AUDIT SUMMARY: LoC: Internet Billing 
AUDIT DETAIL: LoC: Internet Billing 
2020-09-19 03:01:25.2918 Warn MBBSEmu.HostProcess.ExportedModules.Majorbbs.f_open Reopened File: c:\modules\muicyber\MUICYBER\DATA\USERS.DAT - most likely a module bug.

Software Information:

tuday2 commented 3 years ago

Adding the crash message:

2020-09-23 06:14:00.0371 Warn MBBSEmu.HostProcess.ExportedModules.Majorbbs.f_open Reopened File: D:\MbbsX64\modules\LRDCYBR\MUICYBER\DATA\USERS.DAT - most likely a module bug.
Unhandled exception. System.Exception: Unable to locate FileStream for 0106:B8DD (Stream: 0000:0000)
   at MBBSEmu.HostProcess.ExportedModules.Majorbbs.fseek()
   at MBBSEmu.HostProcess.ExportedModules.Majorbbs.Invoke(UInt16 ordinal, Boolean offsetsOnly)
   at MBBSEmu.HostProcess.ExecutionUnits.ExecutionUnit.ExternalFunctionDelegate(UInt16 ordinal, UInt16 functionOrdinal)
   at MBBSEmu.HostProcess.ExecutionUnits.ExecutionUnit.Execute(IntPtr16 entryPoint, UInt16 channelNumber, Boolean simulateCallFar, Boolean bypassState, Queue`1 initialStackValues, UInt16 initialStackPointer)
   at MBBSEmu.Module.MbbsModule.Execute(IntPtr16 entryPoint, UInt16 channelNumber, Boolean simulateCallFar, Boolean bypassSetState, Queue`1 initialStackValues, UInt16 initialStackPointer)
   at MBBSEmu.HostProcess.MbbsHost.CallModuleRoutine(String routine, Action`1 preRunCallback, UInt16 channel)
   at MBBSEmu.HostProcess.MbbsHost.DoNightlyCleanup()
   at MBBSEmu.HostProcess.MbbsHost.WorkerThread()
   at System.Threading.ThreadHelper.ThreadStart_Context(Object state)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.ThreadHelper.ThreadStart()
tuday2 commented 3 years ago

Confirmed still happening on Build 100720-7

enusbaum commented 3 years ago

@tuday2 Was this fixed with #372 ?

tuday2 commented 3 years ago

https://github.com/swenson/lords-of-cyberspace/blob/77c0a05a5e092d2501890c3e5325a10a9f2014ff/cyber/major.c#L2444

According to this github/source ...

Here is the cleanup routine:

/**************************************************************************
    clean up routine, called once daily (generally 3:00 AM)
*/
void cyber_clean(void)
{

    setmbk(glob->cfg);
    NOTIFY("LoC: Clean Up Started");

    glob->shutdown = TRUE;
    glob->server = TRUE;
    Port_Check();
    glob->server = FALSE;

    if (glob->inactdys)
    {
        NOTIFY("LoC: Clearing Inactive Chars"); 
        Clear_Old_Characters();
    }

    NOTIFY("LoC: Checking Tournament"); 
    Tourney_Check(); 
    NOTIFY("LoC: Internet Billing"); 
    Internet_Billing();
    NOTIFY("LoC: Gang Scoring"); 
    Gang_Scoring(); 
    NOTIFY("LoC: Clean Up Finished");
    rstmbk();
}
tuday2 commented 3 years ago

Since we are crashing before it says "Gang Scoring" in the audit trail, it is the last part of the Internet_Billing() method.

tuday2 commented 3 years ago

Internet_Billing()

/****************************************************************************
    during cleanup, process everyone's internet bill
*/
short Internet_Billing(void)
{
    short   n;
    short   index = -1;
    short   written;
    FILE    *fp;

    fp = fopen(USER_FILE, READB);

    do
    {
        ++index;
        fseek(fp, (long) index * HUMAN_SIZE, SEEK_SET);
        n = fread(a_human, HUMAN_SIZE, 1, fp);

        if (n == 1 && !sameas(a_human->userid, OPEN_RECORD))
        {
            a_human->invite = 0;

            if (a_human->gang_pts_today && a_human->gang)
            {
                Read_Gang(a_human->gang);
                ++gang->scorers;
                Write_Gang(a_human->gang);
            }

            a_human->gang_pts_today = 0;
            written = FALSE;

            if 
            (
                a_human->internet.owed <= a_human->bank.balance || 
                a_human->perm[FREE_PERM]
            )
            {
                if (!a_human->perm[FREE_PERM] && a_human->internet.owed)
                {
                    Add_Bank_Record
                    (
                        a_human,
                        -(a_human->internet.owed),
                        -1, -1,
                        "AUTOMATED",
                        "Internet Bill"
                    );
                    written = TRUE;
                }

                if (a_human->internet.credit_rating < 10)
                {
                    ++a_human->internet.credit_rating;
                }

                if (a_human->internet.owed)
                {
                    Add_Internet_Record
                    (
                        a_human,
                        -(a_human->internet.owed),
                        -1, -1,
                        "AUTOMATED",
                        "Bill Paid"
                    );
                    written = TRUE;
                }
            }
            else
            {
                if (a_human->internet.credit_rating)
                {
                    --a_human->internet.credit_rating;
                }

                if (a_human->bank.balance)
                {
                    Add_Internet_Record
                    (
                        a_human,
                        -(a_human->bank.balance),
                        -1, -1,
                        "AUTOMATED",
                        "Bill Paid"
                    );

                    Add_Bank_Record
                    (
                        a_human,
                        -(a_human->bank.balance),
                        -1, -1,
                        "AUTOMATED",
                        "Internet Bill"
                    );

                    written = TRUE;
                }
                else
                {
                    Add_Internet_Record
                    (
                        a_human,
                        0L,
                        -1, -1,
                        "AUTOMATED",
                        "Delinquent!"
                    );

                    written = TRUE;
                }
            }

            if (!written)
            {
                Add_Internet_Record
                (
                    a_human,
                    0L,
                    -1, -1,
                    "AUTOMATED",
                    "No Billing Today"
                );
            }
        }
    }
    while (n == 1);

    fclose(fp);
    EXIT0;
}
synacktic commented 3 years ago

Ok, the issue here is this: During cleanup it opens the USERS.DAT file rb and loops through the users. If it needs to update a user it calls a function which opens it again wb and then updates their records, closes the file handle, returns to the main loop which seeks to the next user until all users are handled and then it closes the original file handle.

Since the called function closes the file and currently that closes the file stream for that file, when it returns to the loop and tries to seek, we crash.

So, we need a way to keep track of references or some such to a given file.

I created a simple test to demonstrate that and it does fix the problem. This can be seen in #412 . If we can find a sane way to do this and fix up the tests to match, then we should be gtg.

enusbaum commented 3 years ago

Fixed with #412