`mllp.ReadMsg` may drop data

[aeqz]

Currently, the ReadMsg function from the mllp package takes an io.Reader argument and wraps it into a bufio.Reader in order to read until the message end delimiter. When doing this, data beyond the end delimiter may have been read from the argument reader and be left in the created reader buffer, resulting in the remaining messages being dropped or truncated:

func TestReadTwoMessages(t *testing.T) {
    reader := strings.NewReader("\x0bFOO\x1c\x0d\x0bBAR\x1c\x0d")

    msg, err := mllp.ReadMsg(reader)
    if err != nil {
        t.Fatalf("Expected FOO, got %v", err)
    }

    if string(msg) != "FOO" {
        t.Fatalf("Expected FOO, got %s", string(msg))
    }

    msg, err = mllp.ReadMsg(reader)
    if err != nil {
        t.Fatalf("Expected BAR, got %v", err) // <- Expected BAR, got EOF
    }

    if string(msg) != "BAR" {
        t.Fatalf("Expected BAR, got %s", string(msg))
    }

    _, err = mllp.ReadMsg(reader)
    if err != io.EOF {
        t.Fatalf("Expected EOF, got %v", err)
    }
}

The previous test succeeds if we wrap the argument beforehand, because bufio.NewReader returns the passed reader if it is already a bufio.Reader with enough size (preventing mllp.ReadMsg from creating a new buffer):

reader := bufio.NewReader(strings.NewReader("\x0bFOO\x1c\x0d\x0bBAR\x1c\x0d"))

Apart from this simple example, I have also seen this happening in a more realistic situation when reading messages from net.Conns accepted from a TCP net.Listener.

Should mllp.ReadMsg take a bufio.Reader rather than creating one in order to prevent this issue? If that is not possible for some reason (I understand that it would be a breaking change), is this already documented somewhere to let the users of this package know how to safely use this function? (I failed to find it if that's the case!).

[nikklassen]

Thanks for your detailed issue (and test!). This definitely sounds like something that should be fixed. We don't version this project (it's only really used as a standalone application), so I'm not worried about changing the type of the arguments.

[aeqz]

That's interesting. I happen to be using it as a Go package for reading and writing messages over MLLP. I'm glad to hear that the function signature change can be considered as a solution!

[nikklassen]

Fixed in 903a912c95ba1a33b62100b3c8a4486aa6b37b9c