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

Fixes all statement leaks in the driver. #455

Merged
merged 6 commits into from
Aug 28, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1399,11 +1399,12 @@ public NClob getNClob(String parameterName) throws SQLException {
*/
/* L3 */ private int findColumn(String columnName) throws SQLServerException {
if (paramNames == null) {
SQLServerStatement s = null;
try {
// Note we are concatenating the information from the passed in sql, not any arguments provided by the user
// if the user can execute the sql, any fragments of it is potentially executed via the meta data call through injection
// is not a security issue.
SQLServerStatement s = (SQLServerStatement) connection.createStatement();
s = (SQLServerStatement) connection.createStatement();
ThreePartName threePartName = ThreePartName.parse(procedureName);
StringBuilder metaQuery = new StringBuilder("exec sp_sproc_columns ");
if (null != threePartName.getDatabasePart()) {
Expand Down Expand Up @@ -1440,6 +1441,10 @@ public NClob getNClob(String parameterName) throws SQLException {
catch (SQLException e) {
SQLServerException.makeFromDriverError(connection, this, e.toString(), null, false);
}
finally {
if (null != s)
s.close();
}
}

int l = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,12 +244,12 @@ private SQLServerResultSet getResultSetFromInternalQueries(String catalog,
SQLServerResultSet rs = null;
try {
rs = ((SQLServerStatement) connection.createStatement()).executeQueryInternal(query);

}
finally {
if (null != orgCat) {
if (null != rs)
rs.close();
Copy link
Contributor

@xiangyushawn xiangyushawn Aug 18, 2017

Choose a reason for hiding this comment

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

this method returns the resultset, so we should not close this resultset here

if (null != orgCat)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we always omit the bracket when we can, so i did the same here.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually we started adding back the brackets even with one line of code, so lets keep the brackets here 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

connection.setCatalog(orgCat);
}
}
return rs;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,8 @@ private void checkClosed() throws SQLServerException {
assert null != st;
stmtParent = st;
con = st.connection;
SQLServerStatement s = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i guess s doesnt need to be instantiated to null but i did it for consistency

SQLServerStatement stmt = null;
if (logger.isLoggable(java.util.logging.Level.FINE)) {
logger.fine(toString() + " created by (" + st.toString() + ")");
}
Expand All @@ -571,7 +573,7 @@ private void checkClosed() throws SQLServerException {
// If the CallableStatement/PreparedStatement is a stored procedure call
// then we can extract metadata using sp_sproc_columns
if (null != st.procedureName) {
SQLServerStatement s = (SQLServerStatement) con.createStatement(ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_READ_ONLY);
s = (SQLServerStatement) con.createStatement(ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_READ_ONLY);
String sProc = parseProcIdentifier(st.procedureName);
if (con.isKatmaiOrLater())
rsProcedureMeta = s.executeQueryInternal("exec sp_sproc_columns_100 " + sProc + ", @ODBCVer=3");
Expand Down Expand Up @@ -659,13 +661,11 @@ private void checkClosed() throws SQLServerException {

String tablesAndJoins = sbTablesAndJoins.toString();

Statement stmt = con.createStatement();
stmt = (SQLServerStatement) con.createStatement();
String sCom = "sp_executesql N'SET FMTONLY ON SELECT " + columns + " FROM " + tablesAndJoins + " '";

ResultSet rs = stmt.executeQuery(sCom);
parseQueryMetaFor2008(rs);
stmt.close();
rs.close();
}
}
}
Expand All @@ -679,6 +679,12 @@ private void checkClosed() throws SQLServerException {
catch(StringIndexOutOfBoundsException e){
SQLServerException.makeFromDriverError(con, stmtParent, e.toString(), null, false);
}
finally {
if (null != s)
s.close();
if (null != stmt)
stmt.close();
}
}

public boolean isWrapperFor(Class<?> iface) throws SQLException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1020,10 +1020,11 @@ else if (resultSet != null) {
/* L0 */ private ResultSet buildExecuteMetaData() throws SQLServerException {
String fmtSQL = userSQL;

SQLServerStatement stmt = null;
ResultSet emptyResultSet = null;
try {
fmtSQL = replaceMarkerWithNull(fmtSQL);
SQLServerStatement stmt = (SQLServerStatement) connection.createStatement();
stmt = (SQLServerStatement) connection.createStatement();
emptyResultSet = stmt.executeQueryInternal("set fmtonly on " + fmtSQL + "\nset fmtonly off");
}
catch (SQLException sqle) {
Expand All @@ -1035,6 +1036,10 @@ else if (resultSet != null) {
SQLServerException.makeFromDriverError(connection, this, form.format(msgArgs), null, true);
}
}
finally {
if (null != stmt)
stmt.close();
Copy link
Contributor

@xiangyushawn xiangyushawn Aug 18, 2017

Choose a reason for hiding this comment

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

this statement creates resultset which will be returned, closing this statement also closes the returned resultset, which is not what we want

}
return emptyResultSet;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,7 @@ private String typeDisplay(int type) {
case XA_START:

if (!serverInfoRetrieved) {
Statement stmt = null;
try {
serverInfoRetrieved = true;
// data are converted to varchar as type variant returned by SERVERPROPERTY is not supported by driver
Expand All @@ -453,7 +454,7 @@ private String typeDisplay(int type) {
+ " convert(varchar(100), SERVERPROPERTY('ProductVersion')) as version,"
+ " SUBSTRING(@@VERSION, CHARINDEX('<', @@VERSION)+2, 2)";

Statement stmt = controlConnection.createStatement();
stmt = controlConnection.createStatement();
ResultSet rs = stmt.executeQuery(query);
rs.next();

Expand All @@ -475,14 +476,23 @@ else if (-1 != version.indexOf('.')) {
ArchitectureOS = Integer.parseInt(rs.getString(4));

rs.close();
stmt.close();
}
// Got caught in static analysis. Catch only the thrown exceptions, do not catch
// run time exceptions.
catch (Exception e) {
if (xaLogger.isLoggable(Level.WARNING))
xaLogger.warning(toString() + " Cannot retrieve server information: :" + e.getMessage());
}
finally {
if (null != stmt)
try {
stmt.close();
}
catch (SQLException e) {
if (xaLogger.isLoggable(Level.FINER))
xaLogger.finer(toString());
}
}
}

sContext = "START:";
Expand Down