Title | Timing error in VersioningFileProvider.getPageProvider(String) |
Date | 19-Feb-2006 20:49:23 EET |
Version | HEAD (rev 1.17) |
Submitter | BobKerns |
Bug criticality | BadBug |
Browser version | |
Bug status | ClosedBug |
PageProvider used | VersioningFileProvider |
Servlet Container | |
Operating System | |
URL | |
Java version |
I'm labeling this as BadBug, because there may be potential for data loss, depending on what use is being made of the properties...But I spotted this by inspection, not experience; it's probably a low-frequency event with a low probability of actual harm.
Still, it's an easy fix.
These lines have a timing hole:
if( m_cachedProperties != null && m_cachedProperties.m_page.equals(page) && m_cachedProperties.m_lastModified == lastModified) { return m_cachedProperties.m_props; } ...
Later it does:
... CachedProperties cp = new CachedProperties(); cp.m_page = page; cp.m_lastModified = lastModified; cp.m_props = props; m_cachedProperties = cp; // Atomic
Note the comment on the write (Atomic), and the lack of atomic read. There being four reads, there are three points of failure, with four different scenarios:
- NullPointerException -- not possible, since it never becomes null once set.
- Comparing lastModified for a different page
- False positive match (requires major timing coincidence on lastModified): wrong properties
- Cache miss due to change after first test: harmless
- Returning different properties than were compared for page/lastModified: wrong properties
To fix, just do the same thing for the read.
CachedProperties cp = m_cachedProperties; // Atomic read if( cp != null && cp.m_page.equals(page) && cp.m_lastModified == lastModified) { return cp.m_props; } ... cp = new CachedProperties(); cp.m_page = page; cp.m_lastModified = lastModified; cp.m_props = props; m_cachedProperties = cp; // Atomic write
Good catch, thanks! I'll look into it.
Fixed in 2.3.81
Add new attachment
Only authorized users are allowed to upload new attachments.