On Sat, Jan 3, 2015 at 12:38 AM, tito <farmat...@tiscali.it> wrote: > On Friday 02 January 2015 21:04:04 you wrote: > >> On Mon, Dec 29, 2014 at 10:19 PM, tito <farmat...@tiscali.it> wrote: > >> > after putting more work at this malloced libpwdgrp functions I think >> > they now are > >> > pretty robust, with no memory leaks and able to handle more or less >> > gracefully > >> > problematic passwd/group/shadow files. The main features of this > >> > implemantation are: > >> > > >> > * 1) the buffer for getpwuid, getgrgid, getpwnam, getgrnam is >> > dynamically > >> > * allocated and reused by later calls. if ERANGE error pops up it is > >> > * reallocated to the size of the longest line found so far in the > >> > * passwd/group files and reused for later calls. > >> > * If ENABLE_FEATURE_CLEAN_UP is set the buffers are freed at program > >> > * exit using the atexit function to make valgrind happy. > >> > * 2) the passwd/group files: > >> > * a) must contain the expected number of fields (as per count of field > >> > * delimeters ":") or we will complain with a error message. > >> > * b) leading or trailing whitespace in fields is allowed and handled. > >> > * c) some fields are not allowed to be empty (e.g. username, uid/gid, > >> > * homedir, shell) and in this case NULL is returned and errno is > >> > * set to EINVAL. This behaviour could be easily changed by > >> > * modifying PW_DEF, GR_DEF, SP_DEF strings (uppercase > >> > * makes a field mandatory). > >> > * d) the string representing uid/gid must be convertible by strtoXX > >> > * functions or NULL is returned and errno is set to EINVAL. > >> > * e) leading or trailing whitespaces in member names and empty members > >> > * are allowed and handled. > >> > * 3) the internal function for getgrouplist uses a dynamically allocated > >> > * buffer and retries with a bigger one in case it is to small; > >> > * 4) the _r functions use the user supplied buffers that are never >> > reallocated > >> > * but use mostly the same common code as the other functions. > >> > * 5) at the moment only the functions really used by busybox code are > >> > * implemented, if you need a particular missing function it should be > >> > * easy to write it by using the internal common code. > >> > > >> > I've done some testing and everything seems to work as expected > >> > nonetheless some more testing by the list members and a good > >> > code review by more experienced programmers is needed as this > >> > lib calls stuff is very new for me and maybe I overlooked > >> > something obvious. > >> > Attached you will find a patch and a drop in replacement > >> > for pwd_grp.c for easier review and testing. > >> > > >> > Size increase is about 120 bytes due to the increased feature set, > >> > but the bloat-o-meter output looks somewhat suspicious: > >> > > >> > ./scripts/bloat-o-meter busybox_unstripped_original busybox_unstripped > >> > function old new delta > >> > convert_to_struct - 351 +351 > >> > parse_common - 214 +214 > >> ... > >> > bb__pgsreader 194 - -194 > >> > bb__parsegrent 246 - -246 > >> > >> > ------------------------------------------------------------------------------ > >> > (add/remove: 16/10 grow/shrink: 4/6 up/down: 1411/-1293) Total: 118 >> > bytes > >> > >> > >> I see the following bloatcheck: > >> > >> function old new delta > >> convert_to_struct - 369 +369 > >> parse_common - 236 +236 > >> tokenize - 133 +133 > >> getgrouplist_internal 195 328 +133 > >> getpw_common - 97 +97 > >> getgr_common - 97 +97 > >> getpw_common_malloc - 81 +81 > >> getgr_common_malloc - 81 +81 > >> parse_file - 76 +76 > >> bb_internal_getpwent_r 102 148 +46 > >> bb_internal_getgrent_r 102 148 +46 > >> my_pwd - 28 +28 > >> my_grp - 16 +16 > >> sp_def - 4 +4 > >> pwd_buffer_size - 4 +4 > >> pwd_buffer - 4 +4 > >> pw_def - 4 +4 > >> my_pw - 4 +4 > >> my_gr - 4 +4 > >> grp_buffer_size - 4 +4 > >> grp_buffer - 4 +4 > >> gr_def - 4 +4 > >> gr_off 3 4 +1 > >> sp_off 9 8 -1 > >> ptr_to_statics 4 - -4 > >> bb_internal_getpwuid 38 19 -19 > >> bb_internal_getspnam_r 121 96 -25 > >> bb_internal_getgrgid 44 19 -25 > >> bb_internal_getpwnam 38 11 -27 > >> get_S 30 - -30 > >> bb_internal_getgrnam 44 11 -33 > >> bb_internal_fgetpwent_r 51 - -51 > >> bb_internal_fgetgrent_r 51 - -51 > >> bb_internal_getpwuid_r 113 48 -65 > >> bb_internal_getgrgid_r 113 48 -65 > >> bb_internal_getpwnam_r 121 40 -81 > >> bb_internal_getgrnam_r 121 40 -81 > >> bb__parsepwent 110 - -110 > >> bb__parsespent 120 - -120 > >> bb__pgsreader 213 - -213 > >> bb__parsegrent 226 - -226 > >> >> ------------------------------------------------------------------------------ > >> (add/remove: 19/8 grow/shrink: 4/10 up/down: 1476/-1227) Total: 249 bytes > >> text data bss dec hex filename > >> 923471 928 17684 942083 e6003 busybox_old > >> 923708 936 17744 942388 e6134 busybox_unstripped > >> > >> The growth of data and bss needs fixing. Old code contained > >> infrastructure which folded all data into one on-demand allocated area. > >> Let's use a similar trick here? > >> > >> tokenize() fails to strip trailing whitespace from last field. > >> > >> get_record_line() returns ERANGE if buffer is too small. Callers > >> retry when they see that. Why can't get_record_line() itself reallocate > >> the buffer, avoiding retry logic altogether? > >> Sounds suspiciously like our good old xmalloc_fgetline() ;) > >> > >> + if (def[idx] == 'm') { > >> + char *s = (char *) nth_string(buffer, idx); > >> + int i = tokenize(s, ','); > >> + char *p = (char *) nth_string(s, i); > >> + /* Now align (p+1), rounding up. */ > >> + /* Assumes sizeof(char **) is a power of 2. */ > >> + members = (char **)((((intptr_t) p) + sizeof(char **)) > >> + & > >> ~((intptr_t)(sizeof(char **) - 1))); > >> + ((struct group *) result)->gr_mem = members; > >> > >> Looks like "members" now points past buffer's terminating NUL, > >> which may not even be within allocated area. > >> > >> How about attached version? > >> > > > > Hi Denys, > > > > thanks for applying it, I never believed this could get into bb!
New year, new hope? ;o) Joke aside, do not be so humble as you have done quite a bit of work to fix a significant flaw in here. I would like to thank you very much for your contribution trying to make busybox more robust for my initial inquiry! > > I fear I need a few days (or more...) to understand your changes. > > Will run it with the test cases I wrote with my standalone version (if > possible). > > I think we should fix the comments at the start as some now seem outdated. > > I really have lots of questions... > > If I understand it correctly now we use our internal malloced buffer > > also for the _r functions and copy the results to the user supplied buffer? > > Also I wonder about: > > > > while ((buf = xmalloc_fgetline(fp)) != NULL) { > > > > if we use it to allocate a buffer holding the complete line from e.g > /etc/group > > that includes already group members why do we need to realloc it later... > > > > Will look at it tomorrow, there is a lot of new stuff for me to learn ;-) > > > > There were also some improvements I was talking about with > > Harald Becker (prevent flooding terminal and syslog with error messages > > if a line with wrong number of fields is found and corrupted/big sized > > passwd/group files) but more to that later. > > Thanks for your time and effort to improve my code, > > > > Ciao, > > Tito > > > > > > > > > _______________________________________________ > busybox mailing list > busybox@busybox.net > http://lists.busybox.net/mailman/listinfo/busybox _______________________________________________ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox