passwd/group parsing

Pierre A. Humblet
Sun Jan 26 23:27:00 GMT 2003

At 11:55 PM 1/25/2003 -0500, Christopher Faylor wrote:
>>  It comes from the code in next_str. By the way, why do you add 
>>  two \0 in "search" used by strpbrk? The second never has any effect.
>pwdgrp::next_str (char c)
>  char search[] = ":\n\0\0";
>  search[2] = c;
>              ^
>          an additional character to parse for
Yep, but that's the first. 

By the way, we never search for both ":" and ",", so search could be "\n\0"
and the default value of c could be ":". 
And if you replace \n by \0 in add_line, then we only search for ":" or ",",
no need for strpbrk, strchr is enough.
It's actually dangerous to rely on \n, it may not be there. If /etc/group
does not end with a NL or if /etc/group is missing, *lptr (and *next_str
will never be 0 and parse_group enters an infinite loop (verified). 
With your recent changes I don't see how lptr could ever be NULL, so there
is no need for a test in next_str nor in next_num.

So in summary, I suggest replacing \n by \0 in add_line and changing 
next_str as follows (it's also a candidate for in-lining).

char *
pwdgrp::next_str (char c)   [default c is ":"]{
  char *res = lptr;
  char *p = strchr (lptr, c);
  if (p)
       *p = 0;
       lptr = p+1;
     lptr = ""; [Create an artificial EOL] 
  return res;

It's also possible to avoid calling next_str in next_num, 
and at the same time keep a test that Corinna had put to 
verify the proper field termination

pwdgrp::next_num (unsigned long& n)
  char * cp;
  n = strtoul (lptr , &cp, 10);
  return lptr != cp && *cp == ':' && (lptr = ++cp);

I love the way you had to define 3 versions of next_num
to satisfy c++ (but I don't see where the "unsigned int"
case happens). Another way would be to have one function
returning an unsigned long and to put a "bool &" as argument.
>>  I believe the old code was also setting gr_mem to a non-NULL legal 
>>  value in the unlikely event where calloc failed.
>Hmm.  From the linux manpage, it looks like maybe it should be setting ENOMEM
Yes, but that would do little good when we parse the file during startup,
nobody looks at errno and we just try to keep going as well we can.
>however there's no need to change the behavior, so I've put it back.  It
>actually simplifies the code a little.

>I don't remember if I mentioned this before, but merging the code
>between and is one thing that I have wanted to do for
>years.  Corinna got things partway there at my request.  I'm trying to
>get it all of the way now.  Using common routines for parsing these
>files, which use common formats makes a lot of sense, IMO.

Yes, I fully agree. Unifying the code crossed my mind when I worked on it a
couple of months ago, but I didn't want to make "gratuitous" changes. 
I should have raised the issue though.
>Btw, I also took your suggestion and made load not return a boolean.
Thanks for listing me in the ChangeLog for such a small change. 
Would adding the simple test below break anything?
if (!initialized)
  etc_ix = etc::init (etc_ix, pc)

By the way, can you explain the recommended usage of debug_printf, 
and paranoid_printf? I never understood the fine points.

Finally I looked at the new locks. Can you really get away with separate
locks? With two threads, one reading pw, the other reading gr, the
following ordering leads to problem if there is a change in /etc/group:
pw: change possible: false, WaitForSingleObject : true
gr: change_possible: false, WaitForSingleObject ": false
pw: set change_possible to true (too late for gr, /etc/group isn't reread).
(not that I am loosing any sleep over that event).


More information about the Cygwin-developers mailing list