Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

windows tcp bug? #38

Open
tenkavideo opened this issue Nov 12, 2023 · 17 comments
Open

windows tcp bug? #38

tenkavideo opened this issue Nov 12, 2023 · 17 comments

Comments

@tenkavideo
Copy link

tenkavideo commented Nov 12, 2023 •
edited
Loading

Hello, I'm encountering an issue with Ncat on Windows. Whenever I send a "P310000" command or any other page command, the vbit2 application closes the connection, and it becomes impossible to reconnect until the process is restarted
tries on windows 11,telnet,ncat ,putty,windows firewall off
teefax demo pages
default vbit config, plus connection vars on port 5570

this vbit answer to P310000 command
[TCPClient::addChar] finished cmd='7
[PageList::FindPage] Selecting 31000
thx!
@SonnyWalkman
Copy link

SonnyWalkman commented Nov 17, 2023 •
edited
Loading

Hello @tenkavideo and @peterkvt80

I believe the issue lies in the use of this function DieWithError in the TCPClient class immediately terminates the process upon encountering an error, as indicated by the exit(1) call.

I believe this is the reason why Vbit2 is needing a restart after an error occurs because it's dead and unable to listen and establish a new TCP connection.

A more graceful approach might be to handle the error without exiting the entire process, perhaps by issuing a command error dropping the connection. The Handler function is responsible for managing incoming client connections. It reads data from the client socket and processes it. If an the value is <= to zero during data reception (as indicated by recv() returning a negative value), signals TCP connection is disconnect followed by the offending DieWithError function is called, leading to the termination of the Vbit2 process.

Peter,
I recommend a small change to the TCPClient function to handle the error? replace the DieWithError function with a response then drop the connection avoid terminating the entire process.
This approach will enable Vbit2 to simply disconnect the connection.

What tenkavideo is alluding to is an issue with the P command which I can't help with sorry?

The command handler loop in the Handler function may need extra checks against client disconnections and errors. Add additional error handling to manage different types of exceptions or unexpected behaviours from the client side which I haven't looked into? May need to handle malformed requests?

Replace below:-

/** HandleTCPClient
 *  Commands come in here.
 * They need to be accumulated and when we have a complete line, send it to a command interpreter.
 */
void TCPClient::Handler(int clntSocket)
{
    char echoBuffer[RCVBUFSIZE];        /* Buffer for echo string */
    char response[RCVBUFSIZE];
    int recvMsgSize;                    /* Size of received message */
    int i;
    clearCmd();

    /* Send received string and receive again until end of transmission */
    for (recvMsgSize=1;recvMsgSize > 0;)      /* zero indicates end of transmission */
    {
        /* See if there is more data to receive */
        if ((recvMsgSize = recv(clntSocket, echoBuffer, RCVBUFSIZE, 0)) < 0)
            **DieWithError("recv() failed");**
        for (i=0;i<recvMsgSize;i++)
        {
            addChar(echoBuffer[i], response);
            if (*response)
                send(clntSocket, response, strlen(response), 0);
        }
        /* Echo message back to client */
        //if (send(clntSocket, echoBuffer, recvMsgSize, 0) != recvMsgSize)
        //    DieWithError("send() failed");
    }

#ifdef WIN32
    closesocket(clntSocket);
#else
    close(clntSocket);    /* Close client socket */
#endif // WIN32
}

With:


/** HandleTCPClient
 *  Commands come in here.
 * They need to be accumulated and when we have a complete line, send it to a command interpreter. 
 * If the connection receives a negation or zero value with just close the socket, keep the client running. #bugfix
 */
void TCPClient::Handler(int clntSocket)
{
    char echoBuffer[RCVBUFSIZE];
    char response[RCVBUFSIZE];
    int recvMsgSize;

    clearCmd();

    while (true) 
    {
        recvMsgSize = recv(clntSocket, echoBuffer, RCVBUFSIZE, 0);

        if (recvMsgSize < 0) {
            // Handle the error without exiting the process
            // Optionally log the error or send a message back to the client
            std::cerr << "Error in receiving data." << std::endl;
            break;
        } 
        else if (recvMsgSize == 0) {
            // Client has closed the connection
            std::cerr << "Client has closed the connection." << std::endl;
            break;
        }

        // Process the received data
        for (int i = 0; i < recvMsgSize; i++) {
            addChar(echoBuffer[i], response);
            if (*response) {
                send(clntSocket, response, strlen(response), 0);
            }
        }
    }

#ifdef WIN32
    closesocket(clntSocket);
#else
    close(clntSocket);
#endif
}

@tenkavideo
Copy link
Author

tenkavideo commented Nov 17, 2023 •
edited
Loading

thx so much!
the issue in windows is that if for exanple i send "P100" command,vibit try to look for Page 10000 then crash the the vbit
process
u can test like this (after enable command in config) :
use telnet or ncat like telnet/ncat 127.0.0.1 5730
write "P100"
vbit crash

@SonnyWalkman
Copy link

Hopefully, @peterkvt80 or @ZXGuesser could at least patch TCPclient not kill vbit2 process for the disconnect. However, I'd prefer all errors to be handled with a response.

@ZXGuesser
Copy link
Collaborator

This code hasn't been significantly worked on for a very long time and there's been some fairly large changes to the pagelists since then to solve some hard crashes, which may have broken something this code was trying to do.
I would note though that the last commit message for selecting and modifying pages in the command handler reads Not working correctly. so maybe it's always been like this

@tenkavideo
Copy link
Author

tenkavideo commented Nov 17, 2023 via email

@ZXGuesser
Copy link
Collaborator

I believe the command interface is working fine and the call to pageList::Match is crashing

@tenkavideo
Copy link
Author

tenkavideo commented Nov 17, 2023 via email

@SonnyWalkman
Copy link

Hello again @tenkavidee.

Until vbit2 remote 'P command' is working? Have you tried ffmpeg SRT (SubRip) and mux it directly from a file? https://www.bannerbear.com/blog/how-to-add-subtitles-to-a-video-file-using-ffmpeg/

Just a thought? Or are you injecting on the fly?

@tenkavideo
Copy link
Author

tenkavideo commented Nov 17, 2023 via email

@SonnyWalkman
Copy link

SonnyWalkman commented Nov 18, 2023 via email

@tenkavideo
Copy link
Author

Yes very happy for zx work!
i hope can found solution for the tcp cmd issue

@SonnyWalkman
Copy link

SonnyWalkman commented Nov 24, 2023 •
edited
Loading

I believe the command interface is working fine and the call to pageList::Match is crashing

int PageList::Match(char* pageNumber)
{
    int matchCount=0;

    std::cerr << ' [PageList::FindPage] Selecting ' << pageNumber << std::endl;
    int begin=0;
    int end=7;

    for (int mag=begin;mag<end+1;mag++)
    {
        // For each page
        for (std::list<TTXPageStream>::iterator p=_pageList[mag].begin();p!=_pageList[mag].end();++p)
        {
            TTXPageStream* ptr;
            char s[6];
            char* ps=s;
            bool match=true;
            for (ptr=&(*p);ptr!=nullptr;ptr=(TTXPageStream*)ptr->Getm_SubPage()) // For all the subpages in a carousel
            {
                // Convert the page number into a string so we can compare it
                std::stringstream ss;
                ss << std::hex << std::uppercase << std::setw(5) << ptr->GetPageNumber();
                strcpy(ps,ss.str().c_str());
                // std::cerr << "[PageList::FindPage] matching " << ps << std::endl;

                for (int i=0;i<5;i++)
                {
                    if (pageNumber[i]!='*') // wildcard
                    {
                        if (pageNumber[i]!=ps[i])
                        {
                            match=false;
                        }
                    }
                }
            }
            if (match)
            {
                matchCount++;
            }
            ptr->SetSelected(match);
        }
    }
    // Set up the iterator for commands that use pages selected by the Page Identity
    _iterMag=0;
    _iter=_pageList[_iterMag].begin();
    
    return matchCount; // final count
}

pageNumber passed in most likely needs to be checked for range? 100-899? Return error is out of range. @peterkvt80 maybe able to look at why the function crashes?

@tenkavideo
Copy link
Author

tenkavideo commented Nov 24, 2023 via email

@SonnyWalkman SonnyWalkman mentioned this issue Dec 4, 2023
Closed
@ZXGuesser
Copy link
Collaborator

0b8613d fixes the crash in FindPage::Match caused by it accessing a null pointer rather than the subpages it was (presumably) intending to.

There's still a lot of dragons lurking in this code that will crash the process, caused by missing bounds checking and the like.

@SonnyWalkman
Copy link

Thanks for your work @ZXGuesser,
Is there any chance in sorting out Vbit2 terminating on error or client disconnection?

@ZXGuesser
Copy link
Collaborator

I've been unable to replicate a broken socket and whenever I disconnect it goes back to Ready for a client to connect as expected. It would help to know what you actually do to cause the error.

@tenkavideo
Copy link
Author

tenkavideo commented Dec 13, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ZXGuesser @SonnyWalkman @tenkavideo