Re: Internal get{pw,gr}XX calls

["Pierre A. Humblet" <Pierre dot Humblet at ieee dot org>]

Hi Corinna,

At 07:19 PM 11/28/2002 +0100, Corinna Vinschen wrote:
>>      }
>> +  BOOL isuninitialized () const { return state == uninitialized; }
>
>I don't see a need to define isuninitialized().  Or we should also
>define other similar methods.  What we currently have is the operator
>pwdgrp_state() so that we always use conditionals like

I am not an expert on C++ classes. I thought that invoking the
isuninitialized() method would simply return "state == uninitialized"
whereas using  passwd_state <= initializing
would run the code of "operator pwdgrp_state ()". It calls 
etc_changed () and WaitForSingleObject (), which is completely useless
in the context of internal calls that don't reread the files.

>instead, as before.  But if you're more happy with using additional
>methods, feel free to use them, but then define all methods needed as
>
>    passwd_state.isuninitialized()
>    passwd_state.isloaded()

Yes, that's the right way. I will do that.

>> Index: passwd.cc
>> ===================================================================
>>    res.pw_name = grab_string (&buf);
>>    res.pw_passwd = grab_string (&buf);
>>    res.pw_uid = grab_int (&buf);
>> +  if (!*buf)
>> +    return 0;
>>    res.pw_gid = grab_int (&buf);
>>    res.pw_comment = 0;
>>    res.pw_gecos = grab_string (&buf);
>> @@ -129,28 +125,6 @@ class passwd_lock
>
>You said that you did it in a relaxed fashion.  Hmm.  Long hmmmmmm.
>Your implementation allows for passwd entries to be cut (or mutilated)
>after the gid.  That should be ok.  I'm just thinking that we should
>perhaps change grab_int so that we know it got a well formed uid and
>gid field, isn't it?  Shouldn't we check for the stop character like this:
>
>  static int
>  grab_int (char **p)
>  {
>    char *src = *p, *stp;
>    int val = strtol (src, &stp, 10);
>    if (stp == src || !isdigit (*stp))   << *stp WON'T EVER BE A DIGIT,
AFAIK. 
>      while (*src)                       
>	src++;
>    else
>      while (*src && *src != ':')   << DO YOU MEAN stp?
>	src++;
>    if (*src == ':')
>      src++;
>    *p = src;
>    return val;
>  }
>
>That would p move to the trailing \0 as soon as the digit string is invalid
>and so more or less immediately stop to evaluate the passwd string.  The
>
>  if (!*buf)
>
>in parse_pwd should then be moved behind grabbing the gid.  What do you
>think?  (Same for parse_grp, btw.)


I understand your idea, but not the details. See comments in the code above.

Currently parse_pwd doesn't perform any check at all, while
parse_grp does some. As far as I know neither approach has ever led
to complaints.

I was afraid that badly formed passwd entries (e.g. comments) in the file 
could lead to internal pw_passwd fields being empty, and thus to 
security holes. The fields following the gid are less important.

Besides that I am not sure if there is much value in being strict. Also
is there a standard? For example can there be blank spaces between 
the uid digits and the delimiting ":"?  
Feel free to go ahead with more checks. 


>> @@ -166,12 +140,8 @@ read_etc_passwd ()
>>    passwd_lock here (cygwin_finished_initializing);
>> 

>> @@ -183,6 +153,7 @@ read_etc_passwd ()
>>  	  pr.close ();
>>  	  debug_printf ("Read /etc/passwd, %d lines", curr_lines);
>>  	}
>> +      passwd_state = loaded;
>
>Uhm?  That looks incorrect.  It shouldn't enter the initializing state
>if the state is already set to initializing which means another thread
>is currently initializing. (Same in read_etc_group())

I think it's OK. 
However with your previous suggestions the line can be completely removed.
Good style leads to simplicity :)


Instead of sending you patches of all 9 files, I am enclosing incremental
ChangeLog and patches, on top of the previous ones.

Pierre

2002-11-29  Pierre Humblet <pierre.humblet@ieee.org>

	* pwdgrp.h: enum pwdgrp_state, remove initializing, add initialized.
	(pwdgrp_check::pwdgrp_state) Replace by pwdgrp_check::ismustread ().
	(pwdgrp_check::ismustread) Create.
 	(pwdgrp_check::operator =) Delete.
	* passwd.cc (read_etc_passwd): Replace "passwd_state <= " by 
	passwd_state::ismustread (). Remove update of passwd_state.	
	(internal_getpwuid): Replace "passwd_state <= " by 
	passwd_state::ismustread ().
	(internal_getpwnam): Ditto.
	(getpwent): Ditto.
	(getpass): Ditto.
	* grp.cc (read_etc_group): Replace "group_state <= " by 
	group_state::ismustread (). Remove update of group_state.	
	(internal_getgrgid): Replace "group_state <= " by 
	group_state::ismustread ().
	(getgrent32): Ditto.
	(internal_getgrent): Ditto.

Attachment: pwd2.diff
Description: Text document