Re: [CFT] Some updates to libc/rpc (second try)

On Thu, Aug 30, 2012 at 02:37:17AM -0700, Garrett Cooper wrote:
> > Detailed description of mistakes in these files and correct implementation:
> >
> > http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/165710
> 
> A developer at $work (Isilon) developed a slightly simpler patch than
> that based on our custom 7.x sources recently to deal with concurrency
> issues in netconfig. I'll talk with a couple people to see whether or
> not the solution can be contributed back [after some polishing --
> maybe -- and further testing].

Can you post changes and corrected bugs in getnetconfig.c and getnetpath.c
in your (your $work) implementation.

This is the list of changes and corrected bugs in my implementation:

1. __nc_error() does not check return value from malloc() and can
pass NULL pointer to thr_setspecific().

2. setnetconfig() has a race condition with reference counter when
several threads call this function and only one thread successfully
opened database file, while other threads failed in this function.
If that one thread called endnetconfig(), then it can keep cached data
in memory, but all threads will not have opened handles.

3. getnetconfig() should return entries from /etc/netconfig file in
the same order as they are specified according to netconfig(5).
If several threads call getnetconfig() using different handlers,
then entries for each handler will be returned in random order.

4. getnetconfig() has a race condition that can cause NULL pointer
dereference when several threads call this function using one handler.

5. getnetconfig() allows to continue to get entries if database file has
invalid format, because it does not remember previous state.

6. endnetconfig() has a race condition with reference count and
can keep cached data, while all handlers are closed.

7. getnetconfigent() uses getnetconfig() and has the same mistakes,
also this function duplicates code from getnetconfig().

8. getnetconfig() and getnetconfigent() use too much memory for
entry data, each entry require ~1 kbytes of memory, while usually
only 50 bytes is needed.

9. parse_ncp() incorrectly parses flags in netconfig entry and allows
wrong combinations of flags, it does not allow spaces before entry,
does not check number of elements in each netconfig entry, does not
allow empty lines.

10. nc_sperror() is not optimal.

11. dup_ncp() is not optimal, allocates more memory than was used in
the original structure, call strcpy() several times instead of
calling memcpy() one time.

12. setnetpath() is not optimal, e.g. it calls setnetconfig() and then
calls endnetconfig().

13. getnetpath() uses getnetconfig() and getnetconfigent() and has
the same mistakes.

14. getnetpath() has race conditions when several threads call this
function using one handler, as a result there are memory leaks
and not synchronized access with modifications to data if the NETPATH
environment variable is set.

15. _get_next_token() is too complex, incorrectly understand \-sequences.

16. All functions do not specify error code in all possible cases,
so nc_sperror() and nc_perror() functions are useless.

Difference between netconfig.c vs getnetconfig.c and getnetpath.c:

1. __nc_error() was corrected, but its implementation is the same,
this is a standard implementation for thread-specific data handling.
nc_perror() was taken from getnetconfig.c, it cannot be written
in other way.

2. Some errors messages were taken from getnetconfig.c.

3. New nc_parse() (old parse_ncp()) was corrected and optimized a bit,
it just parses white space separated fields in a string.

4. Some variables and macro variables names were taken from getnetconfig.c.

5. All other functions and data structures were rewritten.

Additionally I corrected libc/include/reentrant.h, getnetconfig.3,
and getnetpath.3.