[RFC] malloced getpw/grxxx/_r functions for bb

Wed Aug 20 07:25:43 UTC 2014

On Wed, Aug 20, 2014 at 8:12 AM, tito <farmatito at tiscali.it> wrote:
> On Wednesday 20 August 2014 08:12:08 you wrote:
>> How do you prove that your rewrite from scratch is robust?
> Hi,
> I've done minimal testing through the included test program
> and I've done a few valgrind tet runs on every revision until now.
> Still I cannot prove any robustness. That is why I've posted
> the "idea" and a proof of concept implementation that needs
> more work and cleanup to list where more experienced programmers
> can if they so want review that code and suggest fixes, improvements
> and cleanups or just tell me: "Forget about it".

Well, it is not the "last step" to make it robust. It is an upfront
decision and development paradigm. If you cannot make it robust, it
may cause more harm than what you are trying to resolve. This code has
been in busybox for a while, I assume. You need to make sure that what
you intend to solve does work. I would suggest TDD here. Write test
cases for all the cases that is missing and initially they will fail.
Once you get them all passing, it is time to consider. IMHO, this
should be done before the feature development and internal software
design decision since it well defines what you are trying to achieve.

> Ciao,
> Tito
>
>
>  That's why I post it to the
>> On Mon, Aug 18, 2014 at 8:50 PM, tito <farmatito at tiscali.it> wrote:
>> > On Tuesday 05 August 2014 20:16:04 Denys Vlasenko wrote:
>> >> On Mon, Aug 4, 2014 at 7:06 PM, Laszlo Papp <lpapp at kde.org> wrote:
>> >> > sudo busybox adduser
>> >> > fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> >> > passwd: unknown user
>> >> > fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> >> >
>> >> > Yet, the user is created in /etc/shadow:
>> >> >
>> >> > fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff:!:16286:0:99999:7:::
>> >> >
>> >> > This is at least one issue, but there is another here:
>> >> >
>> >> > sudo busybox deluser
>> >> > fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> >> > deluser: unknown user
>> >> > fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> >>
>> >> Both issues come from the same location in codebase:
>> >> bb__pgsreader() parser drops lines which are longer than its buffer.
>> >>
>> >> Effectively, bbox ignores offending line in /etc/passwd.
>> >>
>> >> > Could you please look into this and potentially fix it? Thanks in advance.
>> >>
>> >> Anyone willing to rewrite getpwnam API to use variable-sized malloced buffer?
>> >
>> > Hi,
>> > i couldn 't resist as I was sure there was a lot to learn and so I've started
>> > to reinvent the wheel. At first I've tried to modify the original bb implementation
>> > but the code with its preprocessor black magic made it difficult so I've started
>> > from scratch.
>> > At the moment I have a proof of concept implementation for:
>> >
>> > getpwuid
>> > getgrgid
>> > getpwnam
>> > getgrnam
>> > getpwuid_r
>> > getgrgid_r
>> > getpwnam_r
>> >
>> > The code is ugly and probably bloated for bb so look at it at your own risk :-)
>> >
>> > Other functions still missing but used in bb:
>> > fgetpwent_r
>> > fgetgrent_r
>> > getspnam_r
>> > setpwent
>> > endpwent
>> > setgrent
>> > endgrent
>> > getpwent_r
>> > getgrent_r
>> > initgroups
>> >
>> > What it does:
>> > 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 _NEEDED_ size and reused for later calls.
>> >
>> > 2) the fields in the passwd files are checked and if empty (in some cases) or containing
>> >     whitespace, etc. a error message about malformed line in file xxxxxx
>> >     is printed so that the user knows instead of being silently ignored.
>> >
>> > 3) the _r functions use the user supplied buffers that are never reallocated
>> >     but use mostly the same common functions.
>> >
>> > 4) there was something........
>> >
>> > Before going further some review and improvements (size reduction, obvious errors, bugs
>> > and other horrors) by the list members is needed  and also some hints if this implementation
>> > makes sense at all or if I should forget about it.
>> >
>> > My biggest problem at the moment is a memory leak in handling the members in
>> > group->gr_mem and I don't see a obvious way to fix it.
>> >
>> > The proof of concept is developed out of tree in the pwdgrp.c file that contains also
>> > a test program to grossly check if everything works. The relevant libbb functions are
>> > copied over to the libbb.c file which is included automatically. You can compile it simply by:
>> >
>> > gcc pwdgrp.c -o test -Wall
>> >
>> >
>> > valgrind of a test run shows:
>> >
>> > =7229== HEAP SUMMARY:
>> > ==7229==     in use at exit: 272 bytes in 4 blocks
>> > ==7229==   total heap usage: 22 allocs, 18 frees, 4,012 bytes allocated
>> > ==7229==
>> > ==7229== 68 bytes in 1 blocks are still reachable in loss record 1 of 4
>> > ==7229==    at 0x4028308: malloc (vg_replace_malloc.c:263)
>> > ==7229==    by 0x402849F: realloc (vg_replace_malloc.c:632)
>> > ==7229==    by 0x80489E4: xrealloc (in /home/tito/Desktop/test)
>> > ==7229==    by 0x8048A5C: xrealloc_vector_helper (in /home/tito/Desktop/test)
>> > ==7229==    by 0x8049584: convert_to_grp (in /home/tito/Desktop/test)
>> > ==7229==    by 0x80498AB: my_getgr_common (in /home/tito/Desktop/test)
>> > ==7229==    by 0x80498FE: my_getgrgid (in /home/tito/Desktop/test)
>> > ==7229==    by 0x8049EBD: main (in /home/tito/Desktop/test)
>> > ==7229==
>> > ==7229== 68 bytes in 1 blocks are definitely lost in loss record 2 of 4
>> > ==7229==    at 0x4028308: malloc (vg_replace_malloc.c:263)
>> > ==7229==    by 0x402849F: realloc (vg_replace_malloc.c:632)
>> > ==7229==    by 0x80489E4: xrealloc (in /home/tito/Desktop/test)
>> > ==7229==    by 0x8048A5C: xrealloc_vector_helper (in /home/tito/Desktop/test)
>> > ==7229==    by 0x804953E: convert_to_grp (in /home/tito/Desktop/test)
>> > ==7229==    by 0x80498AB: my_getgr_common (in /home/tito/Desktop/test)
>> > ==7229==    by 0x80498CD: my_getgrnam (in /home/tito/Desktop/test)
>> > ==7229==    by 0x8049D43: main (in /home/tito/Desktop/test)
>> > ==7229==
>> > ==7229== 68 bytes in 1 blocks are definitely lost in loss record 3 of 4
>> > ==7229==    at 0x4028308: malloc (vg_replace_malloc.c:263)
>> > ==7229==    by 0x402849F: realloc (vg_replace_malloc.c:632)
>> > ==7229==    by 0x80489E4: xrealloc (in /home/tito/Desktop/test)
>> > ==7229==    by 0x8048A5C: xrealloc_vector_helper (in /home/tito/Desktop/test)
>> > ==7229==    by 0x804953E: convert_to_grp (in /home/tito/Desktop/test)
>> > ==7229==    by 0x804976C: getgr_common_r (in /home/tito/Desktop/test)
>> > ==7229==    by 0x80497B9: my_getgrnam_r (in /home/tito/Desktop/test)
>> > ==7229==    by 0x8049E0A: main (in /home/tito/Desktop/test)
>> > ==7229==
>> > ==7229== 68 bytes in 1 blocks are definitely lost in loss record 4 of 4
>> > ==7229==    at 0x4028308: malloc (vg_replace_malloc.c:263)
>> > ==7229==    by 0x402849F: realloc (vg_replace_malloc.c:632)
>> > ==7229==    by 0x80489E4: xrealloc (in /home/tito/Desktop/test)
>> > ==7229==    by 0x8048A5C: xrealloc_vector_helper (in /home/tito/Desktop/test)
>> > ==7229==    by 0x804953E: convert_to_grp (in /home/tito/Desktop/test)
>> > ==7229==    by 0x804976C: getgr_common_r (in /home/tito/Desktop/test)
>> > ==7229==    by 0x8049806: my_getgrgid_r (in /home/tito/Desktop/test)
>> > ==7229==    by 0x8049F84: main (in /home/tito/Desktop/test)
>> > ==7229==
>> > ==7229== LEAK SUMMARY:
>> > ==7229==    definitely lost: 204 bytes in 3 blocks
>> > ==7229==    indirectly lost: 0 bytes in 0 blocks
>> > ==7229==      possibly lost: 0 bytes in 0 blocks
>> > ==7229==    still reachable: 68 bytes in 1 blocks
>> > ==7229==         suppressed: 0 bytes in 0 blocks
>> >
>> >
>> > Ciao,
>> > Tito
>> >
>> >
>> >
>> >
>> >
>> > _______________________________________________
>> > busybox mailing list
>> > busybox at busybox.net
>> > http://lists.busybox.net/mailman/listinfo/busybox
>>
> _______________________________________________
> busybox mailing list
> busybox at busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox