Am 27.07.2011 23:12, schrieb Tito:
> On Wednesday 27 July 2011 22:33:09 Matthias Andree wrote:
>> Am 27.07.2011 21:56, schrieb Tito:
>>
>>> Saying that it does not belong there is not enough, please tell me also
>>> where it should be. Looked like a good place to me. In the same
>>> function we check for illegal chars in usernames. You should also take
>>> into account that busybox does not support conf files for the adduser
>>> applet. Eventually the value could be made a config option (so that it
>>> could be
>>> changed) but it looks like bloat to me. Another way could be to add a define
>>> to libbb.h
>>>
>>> #define MAX_USERNAME_LENGTH 32
>>
>> Alright, IEEE Std. 1003.1-2008 aka Single UNIX™ Specification v4 aka The
>> Open Group Base Specifications Issue 7, already has corresponding
>> definitions.
>>
>> It's available for online reading free of charge after registration at
>> http://pubs.opengroup.org/onlinepubs/9699919799/
>>
>> Basically this standard has headers define LOGIN_NAME_MAX and
>> _POSIX_LOGIN_NAME_MAX, in <limits.h> and <unistd.h>, respectively.
>> These could be used, instead of inventing [y]our own. Be sure to read
>> up on getlogin(), unistd.h, limits.h, sysconf thereabouts in the
>> standards before implementing; the latter _POSIX_ variant is the minimum
>> acceptable length for LOGIN_NAME, including the \0 byte, and currently 9.
>>
>> Inconsistencies will cause arbitrary malfunction, non-portability,
>> maintenance headaches and possibly even in-system incompatibilities.
>> Non-NUL terminated C strings are the least of your worries in that case.
>
> Hi,
> Could this be more acceptable. Could be improved by removing
> the double strlen also the error message could be better.
> Just to see if I overlooked something obvious.
No. The standard demands that, statically, the system defines
LOGIN_NAME_MAX to be >= sysconf(_SC_LOGIN_NAME_MAX). You don't validate
that in applications though. Check the getlogin() documentation in
SUSv4 for details on getlogin_r() use if it matters to use.
The validation of proper conformance, however, belongs in a separate
POSIX conformance test suite---but not into applications.
Also note that any half-witted global optimizer will detect and optimize
the common subexpression (strlen(name) + 1) and eliminate the 2nd
expression.
> void FAST_FUNC die_if_bad_username(const char *name)
> {
> /* Enforce length limits on usernames.
> * LOGIN_NAME_MAX: Maximum length of a login name,
> * including the terminating null byte.
> * Must not be less than _POSIX_LOGIN_NAME_MAX (9).
> */
> if (!name
> || strlen(name) + 1 > sysconf(_SC_LOGIN_NAME_MAX)
> || strlen(name) + 1 < _POSIX_LOGIN_NAME_MAX
> )
This is bogus. You want to allow shorter names (2nd strlen) as pointed
out by Lauri, and you don't want to validate a minimum of the maximum.
The standards conformance test for the OS doesn't belong into run-time
data validation checks. I thing you want instead:
(a) a separate check against NULL with separate error message (providing
that die_if_bad_username() can get called with NULL argument at all)
(b) if (strlen(name) + 1 > LOGIN_NAME_MAX) { ... }
(c) omit the sysconf call and thereabouts. Note that sysconf() can
legally return "-1" for a "no limit" semantic, as written previously.
(d) if you meant to test for "empty string" you need !*name or !name[0]
or name[0] == '\0' instead of !name. The compiler should emit the same
code for everything. Be very careful whether you test the pointer or the
content it's pointing at.
> bb_error_msg_and_die("illegal name length");
> /* 1st char being dash or dot isn't valid: */
> goto skip;
Not pretty. goto is not designed to jump *INTO* loop constructs.
Also note that the character set validation belongs into a function of
its own.
> /* For example, name like ".." can make adduser
> * chown "/home/.." recursively - NOT GOOD
> */
>
> do {
> if (*name == '-' || *name == '.')
> continue;
> skip:
> if (isalnum(*name)
This is bogus and can lead to segfaults through out-of-bounds array
subscripts on systems with signed chars. This needs to be
isalnum((unsigned char)*name). This is true for all toupper/tolower and
is*() functions from <ctype.h> where the argument is as wide as char.
Guys, please read the specs and docs!
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
