[QFJ-852] Missing synchronized blocks discovered in code review. Created: 01/Jul/15 Updated: 04/Jul/15 |
|
Status: | Open |
Project: | QuickFIX/J |
Component/s: | Engine |
Affects Version/s: | 1.6.0 |
Fix Version/s: | None |
Type: | Bug | Priority: | Default |
Reporter: | Nathan Tippy | Assignee: | Unassigned |
Resolution: | Unresolved | Votes: | 0 |
Labels: | sequnece |
Attachments: | quickfixjQFJ-852.patch |
Description |
In the class Session in the following method private boolean verify(Message msg, boolean checkTooHigh, boolean checkTooLow) we find that synchronized (state.getLock()) is used to ensure atomic updates to the multiple fields in the state object. If this is needed then we must also check to ensure partial or dirty reads are avoided. This would happen any place that we call for more than one field of the state and assume the fields remain consistent with one another until we make a second or third request for other fields. Directly after this same block the usage of isChunkedResendRequest, getCurrentEndSeqNo and getEndSeqNo run the risk of partial (dirty) reads because they are out side the sync. This same problem also exists here where it may be the cause of other more serious issues. And to a a lesser degree in |
Comments |
Comment by Nathan Tippy [ 02/Jul/15 ] |
For immediate use I apply this patch file to be safe. However this should be re-visited with a more elegant fix. |
Comment by Christoph John [ 03/Jul/15 ] |
Thanks for the patch! |
Comment by Nathan Tippy [ 04/Jul/15 ] |
No I have not seen any problems but after discovery in code review I was uncomfortable running it without a fix. I have not seen any negative consequences to applying the patch. So it was a defensive move. |