simonvetter / modbus

Go modbus stack (client and server)
MIT License
280 stars 89 forks source link

fix: prevent addition of client connection when modbus server is not started #18

Closed LucEdfsf closed 1 year ago

LucEdfsf commented 1 year ago

On rare circumstances, the modbus server can be stop right after accepting client connection. Therefore we can add this connection to the list while the server is down

In server.go the current code is as follow

        sock, err = ms.tcpListener.Accept()
        if err != nil {
            // if the server has just been stopped, return here
            ms.lock.Lock()
            if !ms.started {
                ms.lock.Unlock()
                return
            }
            ms.lock.Unlock()
            ms.logger.Warningf("failed to accept client connection: %v", err)
            continue
        }
(-> if ms.Stop() is called right there, lock the server, everything goes bad)
        ms.lock.Lock()
        // apply a connection limit
        if uint(len(ms.tcpClients)) < ms.conf.MaxClients {
            accepted    = true
            // add the new client connection to the pool
            ms.tcpClients   = append(ms.tcpClients, sock)
        } else {
            accepted    = false
        }
        ms.lock.Unlock()

I guess we could do

        sock, err = ms.tcpListener.Accept()
        if err != nil {
            ms.logger.Warningf("failed to accept client connection: %v", err)
            continue
        }

        ms.lock.Lock()
        // apply a connection limit
        if ms.started && uint(len(ms.tcpClients)) < ms.conf.MaxClients {
            accepted    = true
            // add the new client connection to the pool
            ms.tcpClients   = append(ms.tcpClients, sock)
        } else {
            accepted    = false
        }
        ms.lock.Unlock()

I am not familiar in github I was not able to open the dedicated branch to fix the issue, it might be clearer in a pull review

simonvetter commented 1 year ago

Hey Lukas, thanks for the bug report!

Nice catch. I just pushed v1.6.0 with a fix.

Mind testing it and report here? If the issue is indeed fixed, I'll let you close this.

LucEdfsf commented 1 year ago

Thank you for the fix, will check that it works, will close when I'm sure it's ok :)

simonvetter commented 1 year ago

I'm going to go ahead and close this as I believe it is fixed, but feel free to reopen if not.

Thanks again for the report!