peterkvt80 / vbit2

Teletext streaming. Generates a T42 teletext stream from teletext pages. This can be used with inserter hardware or a Raspberry Pi to generate a video signal that teletext TVs can decode and display. It can also be used with vbit-iv to generate in-vision teletext on a Linux PC or Raspberry Pi.
161 stars 15 forks source link

windows tcp bug? #38

Open tenkavideo opened 11 months ago

tenkavideo commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

Hello Z Do I think will possible to make it working? R and L command work but the bog issue is that I cannot select a page with P command

Il sab 18 nov 2023, 00:13 ZXGuesser @.***> ha scritto:

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

— Reply to this email directly, view it on GitHub https://github.com/peterkvt80/vbit2/issues/38#issuecomment-1817235630, or unsubscribe https://github.com/notifications/unsubscribe-auth/AZDH7WEAH3SUCMBAGKFTM7DYE7VQ7AVCNFSM6AAAAAA7IJCZA2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJXGIZTKNRTGA . You are receiving this because you were mentioned.Message ID: @.***>

ZXGuesser commented 11 months ago

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

tenkavideo commented 11 months ago

I think so, somebody can fix this part? I need it for send subtitles to a page for give accessibility to a Church channel

Il sab 18 nov 2023, 00:30 ZXGuesser @.***> ha scritto:

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

— Reply to this email directly, view it on GitHub https://github.com/peterkvt80/vbit2/issues/38#issuecomment-1817247631, or unsubscribe https://github.com/notifications/unsubscribe-auth/AZDH7WGKLR3PLWBQ5O2S6Y3YE7XPZAVCNFSM6AAAAAA7IJCZA2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJXGI2DONRTGE . You are receiving this because you were mentioned.Message ID: @.***>

SonnyWalkman commented 11 months ago

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 commented 11 months ago

Hello S I need to do on live events so I need to do it on the fly,so teletext will be perfect for that

Il sab 18 nov 2023, 00:47 Sonny Walkman @.***> ha scritto:

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?

— Reply to this email directly, view it on GitHub https://github.com/peterkvt80/vbit2/issues/38#issuecomment-1817261650, or unsubscribe https://github.com/notifications/unsubscribe-auth/AZDH7WGDLNTHGKY7ZUVN67TYE7ZSDAVCNFSM6AAAAAA7IJCZA2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJXGI3DCNRVGA . You are receiving this because you were mentioned.Message ID: @.***>

SonnyWalkman commented 11 months ago

Hello @t, Ok, yes direct injection as teletext is far easier for your needs.

Here in Australia subtitles are mandatory and injected as teletext on page

  1. I know this by analysing the DVB transport stream in detail on the private PES.

If both the TCPClient handling to stop killing the process and the what ZXGuesser has found with page handler I believe vbit2 will do serve most people’s needs.

Peter could look into sorting the page issues and refactoring the TCPClient function?

I could do it myself however I haven’t set up a dev for C++

I very happy ZXGuesser as added the TS encapsulation!

A BIG Thankyou from me ZXGuesser!!

Sonny

On Sat, 18 Nov 2023 at 10:50 am, Muxertester @.***> wrote:

Hello S I need to do on live events so I need to do it on the fly,so teletext will be perfect for that

Il sab 18 nov 2023, 00:47 Sonny Walkman @.***> ha scritto:

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?

— Reply to this email directly, view it on GitHub https://github.com/peterkvt80/vbit2/issues/38#issuecomment-1817261650,

or unsubscribe < https://github.com/notifications/unsubscribe-auth/AZDH7WGDLNTHGKY7ZUVN67TYE7ZSDAVCNFSM6AAAAAA7IJCZA2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJXGI3DCNRVGA>

. You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/peterkvt80/vbit2/issues/38#issuecomment-1817263180, or unsubscribe https://github.com/notifications/unsubscribe-auth/AM4O2S4QVDXBDYKCY4GEH5DYE7Z25AVCNFSM6AAAAAA7IJCZA2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJXGI3DGMJYGA . You are receiving this because you commented.Message ID: @.***>

tenkavideo commented 11 months ago

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

SonnyWalkman commented 11 months ago

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 commented 11 months ago

Hello Rob Thx for take care of this On depends of number of pages also L and R crash for the same reason Hope can be fix!

Il ven 24 nov 2023, 01:43 Sonny Walkman @.***> ha scritto:

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::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 https://github.com/peterkvt80 maybe able to look at why the function crashes?

— Reply to this email directly, view it on GitHub https://github.com/peterkvt80/vbit2/issues/38#issuecomment-1825025995, or unsubscribe https://github.com/notifications/unsubscribe-auth/AZDH7WDHZTXSNIGE73FHVADYF7UT7AVCNFSM6AAAAAA7IJCZA2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRVGAZDKOJZGU . You are receiving this because you were mentioned.Message ID: @.***>

ZXGuesser commented 10 months ago

0b8613db2d1ec2387c81e41dc40203398002f503 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 commented 10 months ago

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

ZXGuesser commented 10 months ago

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 commented 10 months ago

Hello!

If u can provide the windows binary I can test for u

Da: ZXGuesser @.> Inviato: mercoledì 13 dicembre 2023 12:18 A: peterkvt80/vbit2 @.> Cc: Muxertester @.>; Mention @.> Oggetto: Re: [peterkvt80/vbit2] windows tcp bug? (Issue #38)

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.

— Reply to this email directly, view it on GitHub https://github.com/peterkvt80/vbit2/issues/38#issuecomment-1853729070 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AZDH7WGRXUZM2RGNHEWAVJ3YJGFIHAVCNFSM6AAAAAA7IJCZA2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJTG4ZDSMBXGA . You are receiving this because you were mentioned. https://github.com/notifications/beacon/AZDH7WBYAM22OGDJJ3E7MGDYJGFIHA5CNFSM6AAAAAA7IJCZA2WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTOPWUS4.gif Message ID: @. @.> >