Bug 3989 - NIS automount mapping broken
NIS automount mapping broken
Status: NEW
Product: Samba 3.0
Classification: Unclassified
Component: File Services
3.0.23a
All All
: P3 normal
: none
Assigned To: Samba Bugzilla Account
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-08-02 09:47 UTC by Jon. Hallett
Modified: 2007-07-02 04:57 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jon. Hallett 2006-08-02 09:47:39 UTC
The code for NIS automount mapping has broken somewhere between 3.0.10 and 3.0.23a.  The problem is in lib/util.c's automount_lookup function, in which the critical line

    last_value[nis_result_len] = '\0';

has been deleted, causing a string to be left unterminated.

The diff is below.

Thanks,

Jon. Hallett

*** source/lib/util.c~  Fri Jun 23 14:16:50 2006
--- source/lib/util.c   Wed Aug  2 15:01:59 2006
***************
*** 1428,1433 ****
--- 1428,1434 ----
                                &nis_result, &nis_result_len)) == 0) {
                        fstrcpy(last_key, user_name);
                        pstrcpy(last_value, nis_result);
+                       last_value[nis_result_len] = '\0';
                        strip_mount_options(&last_value);

                } else if(nis_error == YPERR_KEY) {
Comment 1 Jeremy Allison 2006-08-02 18:00:13 UTC
pstrcpy *always* terminates it's target string - it's not possible for it to return a non-null terminated string. Can you examine your problem a little more closely please to determine what the real problem is.
Jeremy.
Comment 2 Jon. Hallett 2006-08-03 04:48:34 UTC
(In reply to comment #1)
> pstrcpy *always* terminates it's target string - it's not possible for it to
> return a non-null terminated string. Can you examine your problem a little more
> closely please to determine what the real problem is.
> Jeremy.

The problem is that the string nis_result, returned through yp_match, is not NUL terminated so pstrcpy can't tell where the meaningful part of it ends; nis_result's length is determined solely by nis_result_len, so we have to stick a NUL in the right place, or else we see spurious characters appearing at the end of %p further down stream.

If you aren't happy with the last_value[nis_result_len] = '\0', which could be a problem if nis_result_len is greater than the size of a pstring, how about replacing the pstrcpy with

StrnCpy(last_value, nis_result, nis_result_len > sizeof(pstring)-1 ? sizeof(pstring)-1 : nis_result_len);

instead?

Thanks,

Jon.
Comment 3 Jon. Hallett 2006-08-03 06:00:50 UTC
(In reply to comment #2)

> The problem is that the string nis_result, returned through yp_match, is not
> NUL terminated so pstrcpy can't tell where the meaningful part of it ends;
> nis_result's length is determined solely by nis_result_len, so we have to stick
> a NUL in the right place, or else we see spurious characters appearing at the
> end of %p further down stream.

Reading the manual page for yp_match, I see that I'm not quite right.  In fact, yp_match does NUL terminate the string, but it also sticks a newline in, too, for no apparent reason.  It is the newline which is causing the problem.

In the light of this, can I suggest that the current 

                        fstrcpy(last_key, user_name);
                        pstrcpy(last_value, nis_result);
                        strip_mount_options(&last_value);

is changed to

                        fstrcpy(last_key, user_name);
                        nis_result[nis_result_len] = '\0';
                        pstrcpy(last_value, nis_result);
                        strip_mount_options(&last_value);

nis_result is a pointer to malloced memory, so the assignment is safe, and of course there are no length worries as nis_result_len is guaranteed to be less than the amount of memory malloced.

Thanks,

Jon.
Comment 4 James R Grinter 2007-07-02 04:57:00 UTC
I've been running with the first suggested fix for some time now: as you say, yp_match puts a newline before the null.

yp_match(3NSL) says:
"... For each outkey and outval, two extra bytes of memory are allocated at the end that contain NEWLINE and null, respectively, but these two bytes are not
reflected in outkeylen or outvallen."

Note that there's another lurking bug in the automount_lookup() function that breaks mapping. If the lookup fails, it blanks the last value but doesn't blank the last key, so the last_* lookup cache is corrupted/inconsistent.

(diff is against my util.c, patched for the yp_match() issue.)

--- util.c      23 Jun 2006 14:51:16 -0000      1.2
+++ util.c      2 Jul 2007 09:49:27 -0000
@@ -1375,6 +1375,7 @@
                        /* If Key lookup fails user home server is not in nis_map 
                                use default information for server, and home directory */
                        last_value[0] = 0;
+                       last_key[0] = 0;
                        DEBUG(3, ("YP Key not found:  while looking up \"%s\" in map \"%s\"\n", 
                                        user_name, nis_map));
                        DEBUG(3, ("using defaults for server and home directory\n"));

(this gets triggered, sporadically, if you have the default 'logon home' and 'logon path' values set in your smb.conf. A lookup for 'nobody', as a result of an IPC$ connection, can fail if you don't have an entry for it in your auto.home map, and then a subsequent lookup of the original user matches last_key[] and returns the null value of last_value[].)