Bug 193584 - [autofs] problem when resolving maps with partial path matches
Summary: [autofs] problem when resolving maps with partial path matches
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Edward Tomasz Napierala
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-12 12:22 UTC by Bjoern A. Zeeb
Modified: 2014-09-28 18:27 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Bjoern A. Zeeb freebsd_committer freebsd_triage 2014-09-12 12:22:39 UTC
When an indirect map returns:

...
/auto/foo/www
/auto/foo/www-bar
/auto/foo/www-baz

and we are looking up www-bar the strncmp in node_find() will match on www already and try to mount that;  if parts of the path are expanded from the key facilitating the & we might try to mount

/auto/foo/www-baz from filer:/totally/different/path/&

I have tried to figure out how to fix that strncmp without much luck running into other assertion failures due to path being an empty string for the initial map lookup it seemed, so I added some extra comparison to the child loop, which should catch all cases still but handling a possible trailing / makes it look ugly.  Trying to avoid a possible strdup() failure, I am using two strlen() and a strncmp() on the longer path; obviously one could also check the lens to be equal first.

Here's a part of the diff with all debugging etc. in it still:

@@ -674,19 +674,41 @@ node_find(struct node *node, const char *path)
 	struct node *child, *found;
 	char *tmp;
 
-	//log_debugx("looking up %s in %s", path, node->n_key);
+	log_debugx("looking up %s in %s", path, node->n_key);
 
 	tmp = node_path(node);
+#if 1
 	if (strncmp(tmp, path, strlen(tmp)) != 0) {
+#else
+	log_debugx("comparing '%s' '%s' %zu %zu", tmp, path, strlen(path), strlen(tmp));
+	if (strlen(tmp) != 0 && strncmp(tmp, path, strlen(tmp)) != 0) {
+#endif
 		free(tmp);
 		return (NULL);
 	}
 	free(tmp);
+	node_print(node);
 
 	TAILQ_FOREACH(child, &node->n_children, n_next) {
 		found = node_find(child, path);
-		if (found != NULL)
-			return (found);
+		if (found != NULL) {
+			size_t pl, tl;
+
+			tmp = node_path(found);
+			log_debugx("found candidate %s %s %s", tmp, path, found->n_key);
+			/* node_path() already removes a possible trainling '/'. */
+			tl = strlen(tmp);
+			pl = strlen(path);
+			if (path[pl-1] == '/')
+				pl--;
+			pl = (pl > tl) ? pl : tl;
+			if (strncmp(tmp, path, pl) == 0) {
+				log_debugx("found match %s %s %s %zu", tmp, path, found->n_key, pl);
+				free(tmp);
+				return (found);
+			}
+			free(tmp);
+		}
 	}
 
 	return (node);

I am sure there is a better, proper solution and I am willing to test.
Comment 1 david 2014-09-22 19:40:59 UTC
Looks as if this explains why I was seeing some odd results: I had started testing to see if we could migrate from using amd(8) to autofs, and I was seeing the contents of /volume/build/ when I was looking for /volume/buildtools/.  That's a bit ... awkward.  (Other than a moderate delay on first reference to an automounted directory -- with >28K lines in the maps, that's not entirely surprising -- it seemed to be working OK other than this ("partial path matches") issue.)

Oh: my testing was on FreeBSD 10.1-BETA2 #21 r271873: Fri Sep 19 09:07:42 PDT 2014 (amd64).

Anyway: Looks as if this is a show-stopper in my case.
Comment 2 Edward Tomasz Napierala freebsd_committer freebsd_triage 2014-09-23 19:13:03 UTC
Guys, can you test 11-CURRENT to see if it's fixed properly?
Comment 3 commit-hook freebsd_committer freebsd_triage 2014-09-23 19:13:06 UTC
A commit references this bug:

Author: trasz
Date: Tue Sep 23 19:12:06 UTC 2014
New revision: 272037
URL: http://svnweb.freebsd.org/changeset/base/272037

Log:
  Fix thinko that, with two map entries like shown below, in that order,
  made automountd(8) mix them up: trying to access the second one would
  trigger mount for the first one.

  foo             host:/foo
  foobar          host:/foobar

  PR:		193584
  MFC after:	3 days
  Sponsored by:	The FreeBSD Foundation

Changes:
  head/usr.sbin/autofs/common.c
Comment 4 Bjoern A. Zeeb freebsd_committer freebsd_triage 2014-09-23 19:31:01 UTC
A slightly different conditions to properly check the escape condition. Updating of my HEAD is on the way; also to pick up the kernel change.  I'll report back, probably tomorrow morning.
Comment 5 Bjoern A. Zeeb freebsd_committer freebsd_triage 2014-09-23 22:56:00 UTC
Works like a charm now!

Thanks a lot for fixing.  Please MFC before BETA3/RC1 :-)
Comment 6 commit-hook freebsd_committer freebsd_triage 2014-09-25 17:39:04 UTC
A commit references this bug:

Author: trasz
Date: Thu Sep 25 17:38:43 UTC 2014
New revision: 272117
URL: http://svnweb.freebsd.org/changeset/base/272117

Log:
  MFC r272037:

  Fix thinko that, with two map entries like shown below, in that order,
  made automountd(8) mix them up: trying to access the second one would
  trigger mount for the first one.

  foo             host:/foo
  foobar          host:/foobar

  PR:		193584
  Approved by:	re (gjb)
  Sponsored by:	The FreeBSD Foundation

Changes:
_U  stable/10/
  stable/10/usr.sbin/autofs/common.c
Comment 7 Edward Tomasz Napierala freebsd_committer freebsd_triage 2014-09-26 08:02:25 UTC
David, can you test it?  The fix will be there in BETA3.
Comment 8 david 2014-09-26 19:18:10 UTC
Sorry for delay; Yes: I was able to test on my test machine at work (now running 10.1-BETA3 @r272180), and this issue appears to be resolved for me.  [I *thought* I had tried hand-applying the patch & rebuilding, but that didn't resolve it -- that's (mostly) why I waited until the MFC.]
Comment 9 Edward Tomasz Napierala freebsd_committer freebsd_triage 2014-09-28 18:27:53 UTC
Thanks!