fixing bad connection error when reading large compressed packets #863

dvilaverde · 2024-05-03T11:17:17Z

This is a fix for #862

jnewmano · 2024-05-04T00:44:05Z

I tried running your fork and got a panic, I'll work on getting a test case that I can share to reproduce.

[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x49d587]

goroutine 1 [running]:
io.ReadAtLeast({0x0, 0x0}, {0xc000000000, 0x4, 0x4000}, 0x4)
        /usr/local/go/src/io/io.go:335 +0x67
github.com/go-mysql-org/go-mysql/packet.(*Conn).copyN(0xc00013e070, {0x8bd0e0, 0xc001f0e030}, {0x7fe96f835c00, 0xc00007a550}, 0x1ac000?)
        /go/pkg/mod/github.com/dvilaverde/[email protected]/packet/conn.go:209 +0x206
github.com/go-mysql-org/go-mysql/packet.(*Conn).ReadPacketTo(0xc00013e070, {0x8bd0e0, 0xc001f0e000})
        /go/pkg/mod/github.com/dvilaverde/[email protected]/packet/conn.go:237 +0xc5
github.com/go-mysql-org/go-mysql/packet.(*Conn).ReadPacketReuseMem(0xc00013e070, {0xc000b8e000, 0x1570cb, 0x1ac000})
        /go/pkg/mod/github.com/dvilaverde/[email protected]/packet/conn.go:121 +0x165
github.com/go-mysql-org/go-mysql/client.(*Conn).readResultRows(0xc0000a0340, 0xc000106060, 0x1)

dvilaverde · 2024-05-04T00:59:19Z

I tried running your fork and got a panic, I'll work on getting a test case that I can share to reproduce.

[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x49d587]

goroutine 1 [running]:
io.ReadAtLeast({0x0, 0x0}, {0xc000000000, 0x4, 0x4000}, 0x4)
        /usr/local/go/src/io/io.go:335 +0x67
github.com/go-mysql-org/go-mysql/packet.(*Conn).copyN(0xc00013e070, {0x8bd0e0, 0xc001f0e030}, {0x7fe96f835c00, 0xc00007a550}, 0x1ac000?)
        /go/pkg/mod/github.com/dvilaverde/[email protected]/packet/conn.go:209 +0x206
github.com/go-mysql-org/go-mysql/packet.(*Conn).ReadPacketTo(0xc00013e070, {0x8bd0e0, 0xc001f0e000})
        /go/pkg/mod/github.com/dvilaverde/[email protected]/packet/conn.go:237 +0xc5
github.com/go-mysql-org/go-mysql/packet.(*Conn).ReadPacketReuseMem(0xc00013e070, {0xc000b8e000, 0x1570cb, 0x1ac000})
        /go/pkg/mod/github.com/dvilaverde/[email protected]/packet/conn.go:121 +0x165
github.com/go-mysql-org/go-mysql/client.(*Conn).readResultRows(0xc0000a0340, 0xc000106060, 0x1)

That would be great. I'll take a look when you provide that.

…pressed packet

dvilaverde · 2024-05-04T02:20:21Z

I've pushed one more change that may address the panic you found in your testing. There was a gap that could cause io.ReadAtLeast to be called with a nil reader when the compressed packet header had an uncompressed length with value zero.

Also not sure why golangci-lint checks are failing now, they pass when i run locally using version 1.57.2. The GitHub Action is using 1.58.0 and when i tried with that version it never completes and seems to consume 60gb+ of my system memory.

jnewmano · 2024-05-04T02:24:38Z

The latest commit fixed the panic I was seeing. Thank you.

dvilaverde · 2024-05-05T22:03:35Z

Fixed the golangci-lint issue as well. The latest version 1.58.0 removed 3 linters, so i removed those from the config file.

lance6716

will review later (on vacation)

lance6716 · 2024-05-06T04:29:56Z

packet/conn.go

+		// if we've read to EOF, and we have compression then advance the sequence number
+		// and reset the compressed reader to continue reading the remaining bytes
+		// in the next compressed packet.
+		if c.Compression != MYSQL_COMPRESS_NONE && rd < bcap &&


I just read the comment of ReadAtLeast, seems for goErrors.Is(err, io.ErrUnexpectedEOF) || goErrors.Is(err, io.EOF) we don't need to check rd < bcap

Yes agree with you after reviewing the ReadAtLeast documentation. I've applied the change suggested.

lance6716 · 2024-05-06T04:33:36Z

packet/conn.go

+			}
+
+			// now read the remaining bytes into the buffer containing the first read bytes
+			rd, err = io.ReadAtLeast(c.currentPacketReader(), buf[rd:], bcap-rd)


should we delete this reading and let the outer loop read it? because it may still meet the EOF error like line 191.

Yes terrific suggestion, i've applied this change.

lance6716 · 2024-05-06T04:37:36Z

packet/conn.go

+	return nil, nil
+}
+
+func (c *Conn) currentPacketReader() io.Reader {


there're

compressedReaderActive bool compressedReader io.Reader

in Conn. Seems we can directly check c.compressedReader == nil as the returned reader for currentPacketReader. And compressedReaderActive always has the same value for c.compressedReader == nil so we can delete it.

When I attempted to delete compressedReaderActive the tests in the client package all began failing when I ran them with compression enabled. I think this is because compressedReaderActive is reset to false in WritePacket after writing the compressed packet. So I don't think I can delete it, or at least I feel deleting it is out of scope for this PR.

I've updated this PR with your suggestion. It was 2am for me and I wasn't thinking clearly, but after more sleep, I realized I could easily remove the compressedReaderActive boolean property from Conn.

take care of your health ❤️

lance6716 · 2024-05-07T07:33:32Z

packet/conn.go

+	return nil, nil
+}
+
+func (c *Conn) currentPacketReader() io.Reader {


take care of your health ❤️

jnewmano · 2024-05-07T17:09:53Z

One of the commits after 22dbb42 has led to broken connections when using compression with the error message invalid compressed sequence 2 != 1

dvilaverde · 2024-05-07T17:16:57Z

One of the commits after 22dbb42 has led to broken connections when using compression with the error message invalid compressed sequence 2 != 1

Do you happen to have a way to reproduce? Any other info that could help in diagnosing the issue?

jnewmano · 2024-05-07T17:19:59Z

One of the commits after 22dbb42 has led to broken connections when using compression with the error message invalid compressed sequence 2 != 1

Do you happen to have a way to reproduce? Any other info that could help in diagnosing the issue?

I'll get more information together and open an issue.

jnewmano · 2024-05-07T18:01:27Z

One of the commits after 22dbb42 has led to broken connections when using compression with the error message invalid compressed sequence 2 != 1

Do you happen to have a way to reproduce? Any other info that could help in diagnosing the issue?

Details posted in issue #871

fixing bad connection error when reading large compressed packets

d082741

dvilaverde mentioned this pull request May 3, 2024

Enabling compression on client connection results in "connection was bad" error #862

Closed

dvilaverde added 5 commits May 3, 2024 07:20

fixing linting errors

88f99a9

minor cleanup and some more comments

4941c07

minor cleanup and some more comments

3f855bf

fixing issue when net_buffer_length=1024

3d097f7

fixing packet reader lookup condition

5895dda

handle possible nil access violation when attempting to read next com…

22dbb42

…pressed packet

removed deprecated linters that no longer exist in golangci-lint 1.58.0

5970c88

lance6716 reviewed May 6, 2024

View reviewed changes

dvilaverde and others added 4 commits May 6, 2024 04:34

addressing PR feedback

e372b06

addressing PR feedback

c84b0fc

removed compressedReaderActive

3848abb

Merge branch 'master' into compressed_packet

c2232da

lance6716 approved these changes May 7, 2024

View reviewed changes

packet/conn.go

return nil, nil

}

func (c *Conn) currentPacketReader() io.Reader {

Copy link

Collaborator

lance6716 May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

take care of your health ❤️

lance6716 merged commit 007f306 into go-mysql-org:master May 7, 2024
13 checks passed

dvilaverde deleted the compressed_packet branch May 7, 2024 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixing bad connection error when reading large compressed packets #863

fixing bad connection error when reading large compressed packets #863

dvilaverde commented May 3, 2024

jnewmano commented May 4, 2024 •

edited

Loading

dvilaverde commented May 4, 2024

dvilaverde commented May 4, 2024 •

edited

Loading

jnewmano commented May 4, 2024

dvilaverde commented May 5, 2024 •

edited

Loading

lance6716 left a comment •

edited

Loading

lance6716 May 6, 2024

dvilaverde May 6, 2024

lance6716 May 6, 2024

dvilaverde May 6, 2024

lance6716 May 6, 2024

dvilaverde May 6, 2024

dvilaverde May 6, 2024

lance6716 May 7, 2024

lance6716 May 7, 2024

jnewmano commented May 7, 2024

dvilaverde commented May 7, 2024

jnewmano commented May 7, 2024

jnewmano commented May 7, 2024

fixing bad connection error when reading large compressed packets #863

fixing bad connection error when reading large compressed packets #863

Conversation

dvilaverde commented May 3, 2024

jnewmano commented May 4, 2024 • edited Loading

dvilaverde commented May 4, 2024

dvilaverde commented May 4, 2024 • edited Loading

jnewmano commented May 4, 2024

dvilaverde commented May 5, 2024 • edited Loading

lance6716 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewmano commented May 7, 2024

dvilaverde commented May 7, 2024

jnewmano commented May 7, 2024

jnewmano commented May 7, 2024

jnewmano commented May 4, 2024 •

edited

Loading

dvilaverde commented May 4, 2024 •

edited

Loading

dvilaverde commented May 5, 2024 •

edited

Loading

lance6716 left a comment •

edited

Loading