Skip to content

Commit

Permalink
Fix bug allowing created elements to reference deleted ones
Browse files Browse the repository at this point in the history
The bug allows a newly-created element to refer to a deleted one
if the transactions for both overlap. Precisely, the issue is that
the check that an element exists does not prevent a concurrent
transaction from altering that row.

Because "deleting" an element in the OSM database does not remove
the row, we cannot rely on FK constraints to ensure the correct
behaviour. Instead, this fix relies on manually locking referenced
elements.

Note that this "fix" is suboptimal, as it does not allow any
updates to the referenced elements. Updates which do not delete
the row could safely be done, but will be prevented.

Also, it's not clear what the negative performance impact of this
change will be.
  • Loading branch information
zerebubuth authored and tomhughes committed Jun 13, 2015
1 parent 891ec3d commit cf6a5c1
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 3 deletions.
6 changes: 4 additions & 2 deletions app/models/relation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -246,8 +246,10 @@ def preconditions_ok?(good_members = [])

# use reflection to look up the appropriate class
model = Kernel.const_get(m[0].capitalize)
# get the element with that ID
element = model.find_by(:id => m[1])
# get the element with that ID. and, if found, lock the element to
# ensure it can't be deleted until after the current transaction
# commits.
element = model.lock("for share").find_by(:id => m[1])

# and check that it is OK to use.
unless element && element.visible? && element.preconditions_ok?
Expand Down
4 changes: 3 additions & 1 deletion app/models/way.rb
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,9 @@ def preconditions_ok?(old_nodes = [])
new_nds = (nds - old_nodes).sort.uniq

unless new_nds.empty?
db_nds = Node.where(:id => new_nds, :visible => true)
# NOTE: nodes are locked here to ensure they can't be deleted before
# the current transaction commits.
db_nds = Node.where(:id => new_nds, :visible => true).lock("for share")

if db_nds.length < new_nds.length
missing = new_nds - db_nds.collect(&:id)
Expand Down

0 comments on commit cf6a5c1

Please sign in to comment.