On Friday 02 January 2015 21:04:04 you wrote:
> On Mon, Dec 29, 2014 at 10:19 PM, tito <[email protected]> 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!
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
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
