Bug 1191 - doubts about view and local zone locking
doubts about view and local zone locking
Product: unbound
Classification: Unclassified
Component: server
Other All
: P5 enhancement
Assigned To: unbound team
Depends on:
  Show dependency treegraph
Reported: 2016-12-16 19:49 CET by JINMEI Tatuya
Modified: 2016-12-21 11:35 CET (History)
2 users (show)

See Also:


Note You need to log in before you can comment on or make changes to this bug.
Description JINMEI Tatuya 2016-12-16 19:49:49 CET
This is just from code inspection, so I may be wrong, but I suspect
locking related to views and their local zones is not sufficient in
some cases:

1. in local_zones_answer(), local_zone 'z' can be used after releasing
   the view lock:
			(z = local_zones_lookup(view->local_zones,
			qinfo->qname, qinfo->qname_len, labs,
			qinfo->qclass))) {
   But shouldn't we actually protect the view until we're done with
   'z'?  Otherwise the view's local zones can be updated via the
   remote control interface in parallel.  It could even be destroyed
   by the view_local_zone_remove command.

2. struct acl_addr holds a pointer to struct view, and it can be
   passed to local_zones_answer(), which seems to assume the passed
   view is valid.  This is probably the case actually today, since, as
   far as I can see, views' vtree is never updated once it's built on
   (re)configuration.  But code comments in view.h seem to intend to
   support cases where a view might even be deleted dynamically:

	 * For the node and name you
	 * need to also hold the views_tree lock to change them (or to
	 * delete this view) */

   If that's the intent, I suspect we can't actually hold a raw
   pointer to view in acl_addr; instead, for example, we'll need to
   store a copy of the view name, and when using the corresponding
   view acquire the views_tree lock, lookup the tree for the view by
   name, use the view and then release the lock.  If such a dynamic
   change to views isn't intended, I'd suggest revising the code
   comment so the assumption is much more clearer.
Comment 1 Ralph Dolmans 2016-12-19 12:34:38 CET

Thank you for the report.

Regarding the first item: Please correct my if I'm wrong, but I don't see an issue here. A read lock is acquired on the local zone before releasing the lock on the view. Before the the removal or type update on the view's local zone, a write lock needs to be acquired. So I think only locking the local zone should be sufficient. Adding new items to the view while the local zone is in use should not be a problem, as long as the zone in use is not altered (which should not happen since it is locked).

Regarding the second item: The comment in view.h is wrong, I removed the view deletion part. If we make view deletion possible in the future, this will indeed need some adjustment. Maybe by storing the view's name, or by storing references to the ACLs in the views.

-- Ralph
Comment 2 JINMEI Tatuya 2016-12-19 20:32:25 CET
Regarding the first point, I was probably overlooking the fact that local_zones_del_zone() acquires the zone's lock.  On re-reading the code I don't see an issue either - sorry for the noise.

As for the second issue, I see that, thanks for the clarification.

Please feel free to close the report.