Date: Fri, 31 Aug 2012 14:06:53 +0300 From: Andrey Simonenko <simon@comsys.ntu-kpi.kiev.ua> To: Garrett Cooper <yanegomi@gmail.com> Cc: Pedro Giffuni <pfg@freebsd.org>, freebsd-current@freebsd.org Subject: Re: [CFT] Some updates to libc/rpc (second try) Message-ID: <20120831110653.GA21089@pm513-1.comsys.ntu-kpi.kiev.ua> In-Reply-To: <CAGH67wTTw-waCMcgNoxCD3Dz4Bevka6yW5TG73_U_pd0pqTB-w@mail.gmail.com> References: <503ED6A1.1060902@FreeBSD.org> <20120830092113.GA27015@pm513-1.comsys.ntu-kpi.kiev.ua> <CAGH67wTTw-waCMcgNoxCD3Dz4Bevka6yW5TG73_U_pd0pqTB-w@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120831110653.GA21089>