Skip to content
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

GetInt32Core throws OverflowException for valid data in prepared statements (regression from 0.57) #1018

Closed
weltkante opened this issue Aug 6, 2021 · 5 comments
Assignees
Labels
Milestone

Comments

@weltkante
Copy link

weltkante commented Aug 6, 2021

I've been intending to update our usage of the MySqlConnector package (we were still on the 0.x versions) but when going to anything newer than 1.2.1 (i.e. anything on the 1.3.x or 1.4.x version series) I'm getting OverflowExceptions from GetInt32Core which I wasn't getting before.

I'm calling MySqlDataReader.GetInt32, the column in the table is int NOT NULL and the row causing the exception contains the value 300 which presumably should fit into an integer (at least it did work in earlier releases of the connector).

When it breaks into the debugger I see GetInt32Core is trying to parse an empty span as UTF8 and when looking up the call stack the row seems to think its a TextRow but its contained data looks binary.

repro code
using System.Data.Common;
using System.IO;
using MySqlConnector;

class Program
{
    static void Main(string[] args)
    {
        var cs = new MySqlConnectionStringBuilder();
        cs.Server = "127.0.0.1";
        cs.Database = "sandbox";

        using var conn = new MySqlConnection(cs.ToString());
        conn.Open();

        PrepareIssue(conn);

        using var cmd = conn.CreateCommand();
        cmd.CommandText = ""
            + " SELECT account, id, name, data1, data2, data3, flags, ts1, ts2, ts3, ts4 "
            + " FROM info "
            + " WHERE id = @id AND ts4 is null ";
        cmd.Prepare();
        cmd.Parameters.Add("@id", MySqlDbType.Int32);
        cmd.Parameters["@id"].Value = 174;

        using var reader = cmd.ExecuteReader();
        if (!reader.Read())
            throw new InvalidDataException("Invalid test data provided.");

        var value = reader.GetInt32(0);
        if (value != 300)
            throw new InvalidDataException("Invalid test data provided.");
    }

    static void PrepareIssue(DbConnection conn)
    {
        using var cmd = conn.CreateCommand();

        cmd.CommandText = "DROP TABLE IF EXISTS info";

        cmd.ExecuteNonQuery();

        cmd.CommandText = "" +
            " CREATE TABLE info (" +
            "   id int NOT NULL AUTO_INCREMENT," +
            "   account int NOT NULL," +
            "   name varchar(32) NOT NULL," +
            "   flags int NOT NULL," +
            "   data1 int NOT NULL," +
            "   data2 int NOT NULL," +
            "   data3 int NOT NULL DEFAULT '1'," +
            "   ts1 timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP," +
            "   ts2 timestamp NULL DEFAULT NULL," +
            "   ts3 timestamp NULL DEFAULT NULL," +
            "   ts4 timestamp NULL DEFAULT NULL," +
            "   PRIMARY KEY (id)" +
            " )";

        cmd.ExecuteNonQuery();

        cmd.CommandText = "INSERT INTO info (id, account, name, flags, data1, data2, data3, ts1, ts2, ts3, ts4)" +
            " VALUES ('174', '300', 'xxxxxxxxxx', '0', '273', '185343498', '14', '2020-01-01 12:34:56', NULL, NULL, NULL)";

        cmd.ExecuteNonQuery();
    }
}
@bgrainger bgrainger added the bug label Aug 9, 2021
@bgrainger
Copy link
Member

Thanks for the code sample! I can reproduce against mysql:8.0.23 in Docker with this code.

@bgrainger
Copy link
Member

Note that if you never had IgnorePrepare=false in your connection string, then MySqlCommand.Prepare was always a no-op: #929

It's possible this bug has been around for much longer but just wasn't exposed because preparing a command did nothing by default in 1.2.1 and earlier.

@weltkante
Copy link
Author

weltkante commented Aug 9, 2021

Thanks for the info, it's an old codebase and I'm coming from 0.35 which didn't have IgnorePrepare. No idea if Prepare did anything in the first place, might just be there because it was used in other connection providers. Anyways, testing with IgnorePrepare=false it seems like the protocol parsing regressed between 0.56 and 0.57, so looks like an old bug indeed.

Either removing the Prepare call or turning IgnorePrepare=true seems to be a valid workaround (obviously at the cost of not using prepared statements).

@weltkante weltkante changed the title GetInt32Core throws OverflowException for valid data (regression from 1.2.x) GetInt32Core throws OverflowException for valid data (regression from 0.57) Aug 9, 2021
@weltkante weltkante changed the title GetInt32Core throws OverflowException for valid data (regression from 0.57) GetInt32Core throws OverflowException for valid data in prepared statements (regression from 0.57) Aug 9, 2021
@bgrainger
Copy link
Member

the row seems to think its a TextRow

That's a vital clue.

MySqlConnector infers whether a result set row is text or binary by looking at the contents:

// this might be a binary row, but it might also be a text row whose first column is zero bytes long; try reading
// the row as a series of length-encoded values (the text format) to see if this might plausibly be a text row
var isTextRow = false;

(I don't recall why it's this way; it seems like it would be better to pass that information along when creating the result set as a result of executing the command.)

In your case, the 46-byte payload just so happens to be interpretable as 11 length-prefixed columns (which matches the number of columns that were selected) so MySqlConnector guesses (incorrectly) that it's text, when it should be interpreted as binary.

The fix here most likely is for the command execution code to pass a flag into the result set of whether it should expect text or binary data (based on whether COM_QUERY or COM_STMT_EXECUTE was sent), then to deserialise the data accordingly.

Thanks again for the concise repro.

@bgrainger bgrainger self-assigned this Aug 10, 2021
bgrainger added a commit that referenced this issue Oct 5, 2021
The code previously tried to infer the type of row based on its contents; it now bases the decision on whether a prepared statement was executed.
@bgrainger bgrainger added this to the 2.0 milestone Oct 16, 2021
@bgrainger
Copy link
Member

Fixed in 1.3.14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants