Skip to content

Commit

Permalink
fix(spans): Catch panic when parsing SQL (#3064)
Browse files Browse the repository at this point in the history 
This PR reapplies #3057, which was
[reverted](#3059) because of
panics.

As a workaround, catch unwind panics so we can collect panic cases and
report them upstream.
  • Loading branch information
@jjbayer
jjbayer authored Feb 8, 2024
1 parent d51280d commit c18b04e
Show file tree
Hide file tree
Showing 5 changed files with 152 additions and 95 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
- Drop spans ending outside the valid timestamp range. ([#3013](https://github.com/getsentry/relay/pull/3013))
- Extract INP metrics from spans. ([#2969](https://github.com/getsentry/relay/pull/2969), [#3041](https://github.com/getsentry/relay/pull/3041))
- Add ability to rate limit metric buckets by namespace. ([#2941](https://github.com/getsentry/relay/pull/2941))
- Upgrade sqlparser to 0.43.1.([#3057](https://github.com/getsentry/relay/pull/3057))
- Implement project scoped cardinality limits. ([#3071](https://github.com/getsentry/relay/pull/3071))

## 24.1.1
Expand Down
12 changes: 7 additions & 5 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 1 addition & 4 deletions relay-event-normalization/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,7 @@ serde = { workspace = true }
serde_json = { workspace = true }
serde_urlencoded = "0.7.1"
smallvec = { workspace = true }
# Use a fork of sqlparser until https://github.com/sqlparser-rs/sqlparser-rs/pull/1065 gets merged:
sqlparser = { git = "https://github.com/getsentry/sqlparser-rs.git", rev = "0bb7ec6ce661ecfc468f647fd1003b7839d37fa6", features = [
"visitor",
] }
sqlparser = { version = "0.43.1", features = ["visitor"] }
thiserror = { workspace = true }
url = { workspace = true }
uuid = { workspace = true }
Expand Down
17 changes: 10 additions & 7 deletions relay-event-normalization/src/normalize/span/description/sql/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ mod tests {
scrub_sql_test!(
strip_prefixes_mysql_generic,
r#"SELECT `table`.`foo`, count(*) from `table` WHERE sku = %s"#,
r#"SELECT foo, count(*) from table WHERE sku = %s"#
r#"SELECT foo, count(*) FROM table WHERE sku = %s"#
);

scrub_sql_test_with_dialect!(
Expand Down Expand Up @@ -581,7 +581,7 @@ mod tests {
scrub_sql_test!(
values_multi,
"INSERT INTO a (b, c, d, e) VALuES (%s, %s, %s, %s), (%s, %s, %s, %s), (%s, %s, %s, %s) ON CONFLICT DO NOTHING",
"INSERT INTO a (..) VALUES (%s) ON CONFLICT DO NOTHING"
"INSERT INTO a (..) VALUES (%s) ON CONFLICT DO NOTHING"
);

scrub_sql_test!(
Expand Down Expand Up @@ -876,13 +876,16 @@ mod tests {
);

scrub_sql_test_with_dialect!(
dont_fallback_to_regex,
replace_into,
"mysql",
// sqlparser cannot parse REPLACE INTO. If we know that
// a query is MySQL, we should give up rather than try to scrub
// with regex
r#"REPLACE INTO `foo` (`a`) VALUES ("abcd1234")"#,
"REPLACE INTO foo (a) VALUES (%s)"
"REPLACE INTO foo (..) VALUES (%s)"
);

scrub_sql_test!(
replace_into_set_dont_panic,
r#"REPLACE INTO `foo` SET x = y"#,
"REPLACE INTO foo SET x = y"
);

scrub_sql_test!(
Expand Down
212 changes: 133 additions & 79 deletions relay-event-normalization/src/normalize/span/description/sql/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ use itertools::Itertools;
use once_cell::sync::Lazy;
use regex::Regex;
use sqlparser::ast::{
AlterTableOperation, Assignment, BinaryOperator, CloseCursor, ColumnDef, Expr, Ident,
ObjectName, Query, Select, SelectItem, SetExpr, Statement, TableAlias, TableConstraint,
AlterTableOperation, Assignment, BinaryOperator, CloseCursor, ColumnDef, Expr, FunctionArg,
Ident, ObjectName, Query, Select, SelectItem, SetExpr, Statement, TableAlias, TableConstraint,
TableFactor, UnaryOperator, Value, VisitMut, VisitorMut,
};
use sqlparser::dialect::{Dialect, GenericDialect};
Expand All @@ -27,6 +27,25 @@ static TABLE_NAME_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"(?i)[0-9a-f]{8,
pub fn parse_query(
db_system: Option<&str>,
query: &str,
) -> Result<Vec<Statement>, sqlparser::parser::ParserError> {
match std::panic::catch_unwind(|| parse_query_inner(db_system, query)) {
Ok(res) => res,
Err(_) => {
relay_log::error!(
tags.db_system = db_system,
query = query,
"parse_query panicked",
);
Err(sqlparser::parser::ParserError::ParserError(
"panicked".to_string(),
))
}
}
}

fn parse_query_inner(
db_system: Option<&str>,
query: &str,
) -> Result<Vec<Statement>, sqlparser::parser::ParserError> {
// See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md#notes-and-well-known-identifiers-for-dbsystem
// https://docs.rs/sqlparser/latest/sqlparser/dialect/fn.dialect_from_str.html
Expand Down Expand Up @@ -198,12 +217,41 @@ impl VisitorMut for NormalizeVisitor {
Self::simplify_table_alias(alias);
}
TableFactor::Pivot {
table_alias,
pivot_alias,
value_column,
alias,
..
} => {
Self::simplify_compound_identifier(value_column);
Self::simplify_table_alias(alias);
}
TableFactor::Function {
name, args, alias, ..
} => {
Self::simplify_compound_identifier(&mut name.0);
for arg in args {
if let FunctionArg::Named { name, .. } = arg {
Self::scrub_name(name);
}
}
Self::simplify_table_alias(alias);
}
TableFactor::JsonTable { columns, alias, .. } => {
for column in columns {
Self::scrub_name(&mut column.name);
}
Self::simplify_table_alias(alias);
}
TableFactor::Unpivot {
value,
name,
columns,
alias,
..
} => {
Self::simplify_table_alias(table_alias);
Self::simplify_table_alias(pivot_alias);
Self::scrub_name(value);
Self::scrub_name(name);
Self::simplify_compound_identifier(columns);
Self::simplify_table_alias(alias);
}
}
ControlFlow::Continue(())
Expand Down Expand Up @@ -311,8 +359,10 @@ impl VisitorMut for NormalizeVisitor {
columns, source, ..
} => {
*columns = vec![Self::ellipsis()];
if let SetExpr::Values(v) = &mut *source.body {
v.rows = vec![vec![Expr::Value(Self::placeholder())]]
if let Some(source) = source.as_mut() {
if let SetExpr::Values(v) = &mut *source.body {
v.rows = vec![vec![Expr::Value(Self::placeholder())]]
}
}
}
// Simple lists of col = value assignments are collapsed to `..`.
Expand Down Expand Up @@ -350,89 +400,93 @@ impl VisitorMut for NormalizeVisitor {
Statement::Close {
cursor: CloseCursor::Specific { name },
} => Self::erase_name(name),
Statement::AlterTable { name, operation } => {
Statement::AlterTable {
name, operations, ..
} => {
Self::simplify_compound_identifier(&mut name.0);
match operation {
AlterTableOperation::AddConstraint(c) => match c {
TableConstraint::Unique { name, columns, .. } => {
if let Some(name) = name {
Self::scrub_name(name);
for operation in operations {
match operation {
AlterTableOperation::AddConstraint(c) => match c {
TableConstraint::Unique { name, columns, .. } => {
if let Some(name) = name {
Self::scrub_name(name);
}
for column in columns {
Self::scrub_name(column);
}
}
for column in columns {
Self::scrub_name(column);
TableConstraint::ForeignKey {
name,
columns,
referred_columns,
..
} => {
if let Some(name) = name {
Self::scrub_name(name);
}
for column in columns {
Self::scrub_name(column);
}
for column in referred_columns {
Self::scrub_name(column);
}
}
}
TableConstraint::ForeignKey {
name,
columns,
referred_columns,
..
} => {
if let Some(name) = name {
Self::scrub_name(name);
TableConstraint::Check { name, .. } => {
if let Some(name) = name {
Self::scrub_name(name);
}
}
for column in columns {
Self::scrub_name(column);
TableConstraint::Index { name, columns, .. } => {
if let Some(name) = name {
Self::scrub_name(name);
}
for column in columns {
Self::scrub_name(column);
}
}
for column in referred_columns {
Self::scrub_name(column);
TableConstraint::FulltextOrSpatial {
opt_index_name,
columns,
..
} => {
if let Some(name) = opt_index_name {
Self::scrub_name(name);
}
for column in columns {
Self::scrub_name(column);
}
}
},
AlterTableOperation::AddColumn { column_def, .. } => {
let ColumnDef { name, .. } = column_def;
Self::scrub_name(name);
}
TableConstraint::Check { name, .. } => {
if let Some(name) = name {
Self::scrub_name(name);
}
AlterTableOperation::DropConstraint { name, .. } => Self::scrub_name(name),
AlterTableOperation::DropColumn { column_name, .. } => {
Self::scrub_name(column_name)
}
TableConstraint::Index { name, columns, .. } => {
if let Some(name) = name {
Self::scrub_name(name);
}
for column in columns {
Self::scrub_name(column);
}
AlterTableOperation::RenameColumn {
old_column_name,
new_column_name,
} => {
Self::scrub_name(old_column_name);
Self::scrub_name(new_column_name);
}
TableConstraint::FulltextOrSpatial {
opt_index_name,
columns,
..
AlterTableOperation::ChangeColumn {
old_name, new_name, ..
} => {
if let Some(name) = opt_index_name {
Self::scrub_name(name);
}
for column in columns {
Self::scrub_name(column);
}
Self::scrub_name(old_name);
Self::scrub_name(new_name);
}
},
AlterTableOperation::AddColumn { column_def, .. } => {
let ColumnDef { name, .. } = column_def;
Self::scrub_name(name);
}
AlterTableOperation::DropConstraint { name, .. } => Self::scrub_name(name),
AlterTableOperation::DropColumn { column_name, .. } => {
Self::scrub_name(column_name)
}
AlterTableOperation::RenameColumn {
old_column_name,
new_column_name,
} => {
Self::scrub_name(old_column_name);
Self::scrub_name(new_column_name);
}
AlterTableOperation::ChangeColumn {
old_name, new_name, ..
} => {
Self::scrub_name(old_name);
Self::scrub_name(new_name);
}
AlterTableOperation::RenameConstraint { old_name, new_name } => {
Self::scrub_name(old_name);
Self::scrub_name(new_name);
}
AlterTableOperation::AlterColumn { column_name, .. } => {
Self::scrub_name(column_name);
AlterTableOperation::RenameConstraint { old_name, new_name } => {
Self::scrub_name(old_name);
Self::scrub_name(new_name);
}
AlterTableOperation::AlterColumn { column_name, .. } => {
Self::scrub_name(column_name);
}
_ => {}
}
_ => {}
}
}

Expand Down

0 comments on commit c18b04e

Please sign in to comment.