Skip to content

Commit

Permalink
Workaround MySql Bug #92954
Browse files Browse the repository at this point in the history 
COM_CHANGE_USER does not work if CLIENT_CONNECT_ATTRS is set.
This applies to MySql prior to 5.7

Changes:
* client won't send CLIENT_CONNECT_ATTRS if `Opts::get_connect_attrs`
  returns `None`

Affected APIs:
* `OptsBuilder::connect_attrs` now takes `Option<HashMap>` - set to `None`
  to opt-out of connect attrs
* `Opts::get_connect_attrs` now returns `Option`

Affected APIs:
  • Loading branch information
@blackbeam
blackbeam committed May 17, 2023
1 parent 5d8c5e1 commit 71a67d9
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 46 deletions.
77 changes: 46 additions & 31 deletions src/conn/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -392,13 +392,7 @@ impl Conn {
UTF8_GENERAL_CI
})
.with_auth_plugin(Some(self.0.auth_plugin.clone()))
.with_connect_attributes(
if self.0.opts.get_connect_attrs().is_empty() {
None
} else {
Some(self.0.opts.get_connect_attrs().clone())
},
),
.with_connect_attributes(self.0.opts.get_connect_attrs().cloned()),
))
.into_owned();
self.write_command_raw(&com_change_user)?;
Expand All @@ -412,6 +406,11 @@ impl Conn {
/// This function will try to invoke COM_RESET_CONNECTION with
/// a fall back to COM_CHANGE_USER on older servers.
///
/// ## Warining
///
/// There is a long-standing bug in mysql 5.6 that kills this functionality in presence
/// of connection attributes (see [Bug #92954](https://bugs.mysql.com/bug.php?id=92954)).
///
/// ## Note
///
/// Re-executes [`Opts::get_init`].
Expand Down Expand Up @@ -448,6 +447,11 @@ impl Conn {
/// * Using non-default `opts` for a pooled connection is discouraging.
/// * Connection options will be updated permanently.
///
/// ## Warining
///
/// There is a long-standing bug in mysql 5.6 that kills this functionality in presence
/// of connection attributes (see [Bug #92954](https://bugs.mysql.com/bug.php?id=92954)).
///
/// [1]: https://dev.mysql.com/doc/c-api/5.7/en/mysql-change-user.html
pub fn change_user(&mut self, opts: ChangeUserOpts) -> Result<()> {
self.exec_com_change_user(opts)
Expand Down Expand Up @@ -714,11 +718,13 @@ impl Conn {
| CapabilityFlags::CLIENT_MULTI_RESULTS
| CapabilityFlags::CLIENT_PS_MULTI_RESULTS
| CapabilityFlags::CLIENT_PLUGIN_AUTH
| CapabilityFlags::CLIENT_CONNECT_ATTRS
| (self.0.capability_flags & CapabilityFlags::CLIENT_LONG_FLAG);
if self.0.opts.get_compress().is_some() {
client_flags.insert(CapabilityFlags::CLIENT_COMPRESS);
}
if self.0.opts.get_connect_attrs().is_some() {
client_flags.insert(CapabilityFlags::CLIENT_CONNECT_ATTRS);
}
if let Some(db_name) = self.0.opts.get_db_name() {
if !db_name.is_empty() {
client_flags.insert(CapabilityFlags::CLIENT_CONNECT_WITH_DB);
Expand All @@ -730,30 +736,34 @@ impl Conn {
client_flags | self.0.opts.get_additional_capabilities()
}

fn connect_attrs(&self) -> HashMap<String, String> {
let program_name = match self.0.opts.get_connect_attrs().get("program_name") {
Some(program_name) => program_name.clone(),
None => {
let arg0 = std::env::args_os().next();
let arg0 = arg0.as_ref().map(|x| x.to_string_lossy());
arg0.unwrap_or_else(|| "".into()).into_owned()
}
};
fn connect_attrs(&self) -> Option<HashMap<String, String>> {
if let Some(attrs) = self.0.opts.get_connect_attrs() {
let program_name = match attrs.get("program_name") {
Some(program_name) => program_name.clone(),
None => {
let arg0 = std::env::args_os().next();
let arg0 = arg0.as_ref().map(|x| x.to_string_lossy());
arg0.unwrap_or_else(|| "".into()).into_owned()
}
};

let mut attrs = HashMap::new();
let mut attrs_to_send = HashMap::new();

attrs.insert("_client_name".into(), "rust-mysql-simple".into());
attrs.insert("_client_version".into(), env!("CARGO_PKG_VERSION").into());
attrs.insert("_os".into(), env!("CARGO_CFG_TARGET_OS").into());
attrs.insert("_pid".into(), process::id().to_string());
attrs.insert("_platform".into(), env!("CARGO_CFG_TARGET_ARCH").into());
attrs.insert("program_name".into(), program_name);
attrs_to_send.insert("_client_name".into(), "rust-mysql-simple".into());
attrs_to_send.insert("_client_version".into(), env!("CARGO_PKG_VERSION").into());
attrs_to_send.insert("_os".into(), env!("CARGO_CFG_TARGET_OS").into());
attrs_to_send.insert("_pid".into(), process::id().to_string());
attrs_to_send.insert("_platform".into(), env!("CARGO_CFG_TARGET_ARCH").into());
attrs_to_send.insert("program_name".into(), program_name);

for (name, value) in self.0.opts.get_connect_attrs().clone() {
attrs.insert(name, value);
}
for (name, value) in attrs.clone() {
attrs_to_send.insert(name, value);
}

attrs
Some(attrs_to_send)
} else {
None
}
}

fn do_ssl_request(&mut self) -> Result<()> {
Expand Down Expand Up @@ -785,7 +795,7 @@ impl Conn {
self.0.opts.get_db_name().map(str::as_bytes),
Some(self.0.auth_plugin.clone()),
self.0.capability_flags,
Some(self.connect_attrs()),
self.connect_attrs(),
);

let mut buf = get_buffer();
Expand Down Expand Up @@ -1889,6 +1899,9 @@ mod test {
.unwrap();
};

conn.query_drop("DELETE FROM mysql.user WHERE user = ''")
.unwrap();
let _ = conn.query_drop("SET GLOBAL secure_auth = 0");
conn.query_drop("FLUSH PRIVILEGES").unwrap();

let mut conn2 = Conn::new(get_opts().secure_auth(false)).unwrap();
Expand Down Expand Up @@ -2340,7 +2353,9 @@ mod test {

#[test]
fn should_set_connect_attrs() {
let opts = OptsBuilder::from_opts(get_opts());
let opts = OptsBuilder::from_opts(
get_opts().connect_attrs::<String, String>(Some(Default::default())),
);
let mut conn = Conn::new(opts).unwrap();

let support_connect_attrs = match (conn.0.server_version, conn.0.mariadb_server_version)
Expand Down Expand Up @@ -2401,7 +2416,7 @@ mod test {
connect_attrs.insert("foo", "foo val");
connect_attrs.insert("bar", "bar val");
connect_attrs.insert("program_name", "my program name");
let mut conn = Conn::new(opts.connect_attrs(connect_attrs)).unwrap();
let mut conn = Conn::new(opts.connect_attrs(Some(connect_attrs))).unwrap();
expected_values.pop(); // remove program_name at the last
expected_values.push(("foo", "foo val"));
expected_values.push(("bar", "bar val"));
Expand Down
49 changes: 35 additions & 14 deletions src/conn/opts/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ pub(crate) struct InnerOpts {
additional_capabilities: CapabilityFlags,

/// Connect attributes
connect_attrs: HashMap<String, String>,
connect_attrs: Option<HashMap<String, String>>,

/// Disables `mysql_old_password` plugin (defaults to `true`).
///
Expand Down Expand Up @@ -266,7 +266,7 @@ impl Default for InnerOpts {
stmt_cache_size: DEFAULT_STMT_CACHE_SIZE,
compress: None,
additional_capabilities: CapabilityFlags::empty(),
connect_attrs: HashMap::new(),
connect_attrs: Some(HashMap::new()),
secure_auth: true,
enable_cleartext_plugin: false,
#[cfg(test)]
Expand Down Expand Up @@ -450,7 +450,7 @@ impl Opts {
self.0.additional_capabilities
}

/// Connect attributes
/// Connect attributes (the default connect attributes are sent by default).
///
/// This value is sent to the server as custom name-value attributes.
/// You can see them from performance_schema tables: [`session_account_connect_attrs`
Expand All @@ -464,10 +464,18 @@ impl Opts {
///
/// ### Note
///
/// - set `connect_attrs` to `None` to completely remove connect attributes
/// - set `connect_attrs` to an empty map to send only the default attributes
///
/// #### Warning
///
/// > There is a bug in MySql 5.6 that kills COM_CHANGE_USER in the presence of connection
/// > attributes so it's better to stick to `None` for mysql < 5.7.
///
/// Attribute names that begin with an underscore (`_`) are not set by
/// application programs because they are reserved for internal use.
///
/// The following attributes are sent in addition to ones set by programs.
/// The following default attributes are sent in addition to ones set by programs.
///
/// name | value
/// ----------------|--------------------------
Expand All @@ -482,8 +490,8 @@ impl Opts {
/// [`performance_schema`]: https://dev.mysql.com/doc/refman/8.0/en/performance-schema-system-variables.html#sysvar_performance_schema
/// [`performance_schema_session_connect_attrs_size`]: https://dev.mysql.com/doc/refman/en/performance-schema-system-variables.html#sysvar_performance_schema_session_connect_attrs_size
///
pub fn get_connect_attrs(&self) -> &HashMap<String, String> {
&self.0.connect_attrs
pub fn get_connect_attrs(&self) -> Option<&HashMap<String, String>> {
self.0.connect_attrs.as_ref()
}

/// Disables `mysql_old_password` plugin (defaults to `true`).
Expand Down Expand Up @@ -982,7 +990,7 @@ impl OptsBuilder {
self
}

/// Connect attributes
/// Connect attributes (the default connect attributes are sent by default).
///
/// This value is sent to the server as custom name-value attributes.
/// You can see them from performance_schema tables: [`session_account_connect_attrs`
Expand All @@ -996,10 +1004,18 @@ impl OptsBuilder {
///
/// ### Note
///
/// - set `connect_attrs` to `None` to completely remove connect attributes
/// - set `connect_attrs` to an empty map to send only the default attributes
///
/// #### Warning
///
/// > There is a bug in MySql 5.6 that kills COM_CHANGE_USER in the presence of connection
/// > attributes so it's better to stick to `None` for mysql < 5.7.
///
/// Attribute names that begin with an underscore (`_`) are not set by
/// application programs because they are reserved for internal use.
///
/// The following attributes are sent in addition to ones set by programs.
/// The following default attributes are sent in addition to ones set by programs.
///
/// name | value
/// ----------------|--------------------------
Expand All @@ -1016,14 +1032,19 @@ impl OptsBuilder {
///
pub fn connect_attrs<T1: Into<String> + Eq + Hash, T2: Into<String>>(
mut self,
connect_attrs: HashMap<T1, T2>,
connect_attrs: Option<HashMap<T1, T2>>,
) -> Self {
self.opts.0.connect_attrs = HashMap::with_capacity(connect_attrs.len());
for (name, value) in connect_attrs {
let name = name.into();
if !name.starts_with('_') {
self.opts.0.connect_attrs.insert(name, value.into());
if let Some(connect_attrs) = connect_attrs {
let mut attrs = HashMap::with_capacity(connect_attrs.len());
for (name, value) in connect_attrs {
let name = name.into();
if !name.starts_with('_') {
attrs.insert(name, value.into());
}
}
self.opts.0.connect_attrs = Some(attrs);
} else {
self.opts.0.connect_attrs = None;
}
self
}
Expand Down
3 changes: 2 additions & 1 deletion src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1017,7 +1017,8 @@ macro_rules! def_get_opts {
let database_url = $crate::def_database_url!();
let mut builder =
$crate::OptsBuilder::from_opts($crate::Opts::from_url(&*database_url).unwrap())
.init(vec!["SET GLOBAL sql_mode = 'TRADITIONAL'"]);
.init(vec!["SET GLOBAL sql_mode = 'TRADITIONAL'"])
.connect_attrs::<String, String>(None);
if test_compression() {
builder = builder.compress(Some(Default::default()));
}
Expand Down

0 comments on commit 71a67d9

Please sign in to comment.