[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH] malloced getpw/grxxx functions for bb
From:       Laszlo Papp <lpapp () kde ! org>
Date:       2015-01-03 17:01:48
Message-ID: CAOMwXhNwb81CvAxiOTjSp2YtPv9_1SwqadBL0o15XpMrkHZ7yA () mail ! gmail ! com
[Download RAW message or body]

On Sat, Jan 3, 2015 at 12:38 AM, tito <farmatito@tiscali.it> wrote:
> On Friday 02 January 2015 21:04:04 you wrote:
>
>> On Mon, Dec 29, 2014 at 10:19 PM, tito <farmatito@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
[prev in list] [next in list] [prev in thread] [next in thread]