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

Potential resource leak in DataSourceUtils.doGetConnection [SPR-17559] #22091

Closed
spring-issuemaster opened this issue Dec 3, 2018 · 6 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Dec 3, 2018

Yurx opened SPR-17559 and commented

Method doGetConnection in org.springframework.jdbc.datasource.DataSourceUtils has code that leaks (or has potential to leak) resources.

Objects like logger, DataSource, Connection, etc are used or expected to be referenced between the connection is created and returned. Use of those objects can raise exceptions and cause a connection to be created but not returned from the function. This prevents the caller from being able to release that connection.

The solution is simple and quite standard - try/catch clause is needed, see attachment.

This affects all recent versions including 5.0.x and 5.1.x


Affects: 5.0.11, 5.1.3

Attachments:

Backported to: 5.0.12, 4.3.22

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 3, 2018

Juergen Hoeller commented

As far as I can see, the DataSource and the Connection are not actually used in that phase, just their references passed into objects to be registered with the synchronization manager. This would only fail in case of a hard reoccurring framework bug (assertions etc) which we're generally not using try-catch blocks against.

That said, the use of a logger in such a phase is debatable. No regular logger implementation would fail in such a scenario (it would rather defer/skip writing a log entry), so I don't see a problem in practice here, but defensively speaking we should avoid log statements at such a point.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 3, 2018

Juergen Hoeller commented

I've removed affected debug log statements from DataSourceUtils, EntityManagerFactoryUtils and ConnectionFactoryUtils, leaving just housekeeping which would be fatal to fail in any case.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 3, 2018

Yurx commented

I see connectionHandle calls releaseConnection (in holderToUse.setConnection). This could be overwritten and cause runtime exceptions. I see getTargetDataSource called on DataSource in new ConnectionSynchronization. I see ResourceHolder.setSynchronizedWithTransaction is not final and can be overwritten with a potential of runtime failures. Even if you remove log.debug from that method logging is used inside other methods called from there which may cause failures. To make it short I see not simply one or two simple assignments but a lot of code with a few cases that have real potential to cause resource leak. That is a textbook example of where to use try/catch block.

Thinking defensively try/except/finally concept ensures both current implementation and future code changes are safe. I see this pattern is in use almost everywhere in that module only this code stands out.

I don't see how this can be marked as complete without proper resource management put in place.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 3, 2018

Juergen Hoeller commented

Fair enough, in custom cases there could be unexpected exceptions thrown... even if those probably indicate fatal misbehavior, some such exceptions may be recoverable. A defensive try-catch block doesn't hurt indeed, in addition to reduced log statements (which were inconsistently placed across our different resource utilities anyway).

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 4, 2018

Juergen Hoeller commented

I've added corresponding try-catch blocks to DataSourceUtils and EntityManagerFactoryUtils where we potentially perform external delegation calls which might throw unexpected exceptions after resource retrieval but before returning. I hope this addresses your particular concerns now.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 4, 2018

Yurx commented

Yes sir now it does. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.