-
Notifications
You must be signed in to change notification settings - Fork 68
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.
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
Fix time(0) buffer overwrite #67
base: main
Are you sure you want to change the base?
Conversation
This test currently fails. I can't figure out what the server wants.
thx for the contribution! Have you tried using bcp.exe to create a format file for a table having this column to see what ODBC does with it in native mode? The TDS doc doesn't even list 0 as an option for scale. |
Blind writing to a buffer of unknown size violates every standard of coding I've ever been held to. Refers to: types.go:901 in 01f253b. [](commit_id = 01f253b, deletion_comment = False) |
if the function isn't made safer by checking len(byte), at minimum change this comment to say "5 long" and it could panic if len(buf) is not at least 5. Better to panic with a good message than to have the buffer overrun isn't it? Refers to: types.go:900 in 01f253b. [](commit_id = 01f253b, deletion_comment = False) |
My experience with SQL Server is very limited, so I don't quite understand what you mean by "Have you tried using bcp.exe to create a format file for a table having this column to see what ODBC does with it in native mode?" But as for your comments on the buffer size, I agree 100%. That was my first instinct, and I think that a panic is probably the right move. |
bcp is the standard bulk copy command line tool, it's available with SQL Server on Windows and as a Linux tools package. In reply to: 1311775759 |
My thought on how to proceed from here was to run wireshark to look at the bytes sent over the wire from bcp to an SQL Server instance, to figure out what it does. Is there an easier way to achieve that with bcp, without involving wireshark? |
OK thanks.. I'll try that command first, and see if that gives me any clues. |
you could bcp out the data from the table to a native format data file. Those bytes will match the wire format afaik |
The change in
types.go
fixes a buffer overrun when encoding to atime(0)
field.However, this change still doesn't fix the fact that any
time
field except fortime(7)
causes a BCP protocol failure.In the test that I have added, I create a
time(0)
field, and this causes the test to fail. Note that it is not justtime(0)
that fails. All precision values from 0..6 cause the test to fail.I have read the protocol documentation in https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-tds/ce3183a6-9d89-47e8-a02f-de5a1a1303de and cross-referenced it with the
calcTimeSize
andencodeTimeInt
functions, and the Go code looks correct. I have also tried hardcoding a returned buffer of all sizes from 1 to 5 bytes, but they all yield the same BCP error.In summary, even if nobody can figure out how to make time(0..6) fields work, I think it's still a good idea to merge my change to
types.go
, to fix the buffer overrun.