[Toybox] Update chsh patch

Rob Landley rob at landley.net
Sat Mar 27 05:23:52 PDT 2021


On 3/26/21 6:11 PM, amichaelc via Toybox wrote:
> I had a few (3 maybe?) system calls where I got the error checking backwards, so
> I fixed those. I also realized I forgot to format the code with the 80 character
> thing, so I shortened a bunch of lines.

Sorry, I've already done some cleanups on the file so my version doesn't match
this. (See repo.)

Not complete cleanups, for example I dunno what the "move passwd entry from file
to string" section is trying to accomplish. (You can fmemopen(toybuf,
sizeof(toybuf), "rw") instead of a tmpfile but WHY are you writing to a
tmpfile...? You called realloc() without saving the return pointer?)

Also, there's a constellation of /etc/passwd commands in "pending" (useradd,
userdel, sulogin, groupadd, groupdel) which all use infrastructure in
lib/password.c that could itself use some TLC. (My local copy has "TODO this is
completely broken" right before get_nextcolon(), and several changes to
update_password(). I'm not sure how load-bearing this infrastructure is right
now, but I haven't got a lib/pending...)

> The command works fine except for when I request a shell of 15+ characters (such
> as /usr/bin/xonsh), in which case it segfaults. I'm still working on this, but I
> think the string is too long for the passwd struct (which wouldn't explain why
> `update_passwd("/etc/passwd", user, NULL)` is failing).

I haven't actually tested my changed version yet (just made sure it compiled, it
_is_ still in pending after all: I don't test this sort of thing on my dev
system and I haven't set up a qemu instance to fiddle with it in yet, plus
there's the whole "what should test suite entires for this class of command look
like, especially given that Android doesn't use unix style text files to store
this kind of data, it puts them in the windows registry, which gets us back to
the earlier  "posix build container" issue, which is one reason I haven't
tackled this can of worms yet...)

However, note this change I did:

  -  strncpy(passwd_info->pw_shell, shell, buf_size);
  +  passwd_info->pw_shell = line;

Because in /usr/include/pwd.h:

  /* The passwd structure.  */
  struct passwd
  {
    char *pw_name;                /* Username.  */
    char *pw_passwd;              /* Password.  */
    __uid_t pw_uid;               /* User ID.  */
    __gid_t pw_gid;               /* Group ID.  */
    char *pw_gecos;               /* Real name.  */
    char *pw_dir;                 /* Home directory.  */
    char *pw_shell;               /* Shell program.  */
  };

Which turns out to be posix:

  https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/pwd.h.html

I.E. you're not writing into an array, you're dereferencing a pointer to write
into an allocation of unknown length, which is not ideal.

Rob



More information about the Toybox mailing list