[Toybox] Clean up password.c
Rob Landley
rob at landley.net
Tue Apr 6 04:36:02 PDT 2021
On 4/5/21 6:49 AM, amichaelc wrote:
> Sorry, here they are.
>>> I have four patches. They all depend on each other, and have to be applied
>>> in order, except that you can skip the second if you want.
>>> The first cleans up the style in password.c.
Looking at your first patch, some of the changes are good (removing the time.h
include), and others just seem like churn? Capitalizing the first letter of a
comment? Renaming variables to be dictionary words?
This bit:
+ int i, len, bitpos, bits;
+ char *s;
struct {
char *type, id, len;
} al[] = {{"des", 0, 2}, {"md5", 1, 8}, {"sha256", 5, 16}, {"sha512", 6, 16}};
- int i;
for (i = 0; i < ARRAY_LEN(al); i++) {
- if (!strcmp(algo, al[i].type)) {
- int len = al[i].len;
- char *s = salt;
+ if (!strcmp(algorithm, al[i].type)) {
+ len = al[i].len;
+ s = salt;
...
for (i=0; i<len; i++) {
- int bitpos = i*6, bits = bitpos/8;
+ bitpos = i*6;
+ bits = bitpos/8;
I don't mind variables being declared in blocks other than the outermost, I just
want them declared at the start of a block so you can find them. (Declaring
variables in the middle of a block makes them harder to spot.) You aren't saving
lines when the initialization still has to go in the same place, and in this
case you turned one line into two at the end there, which isn't _bad_ but isn't
an improvement either?
And a few of these tweaks introduce actual bugs, such as:
- int i, tty = tty_fd(), ret = 1;
+ int i, ret, tty = tty_fd();
If buflen is 1 read_password() would now return an uninitialized variable?
Also, I tend to have a blank line before break/continue/return so the flow
control stands out more:
ret = 1;
-
break;
(The exception is when the break/continue/return is on the same line as an if()
statement, which is already flow control. Yeah, mildly inconsistent but it's a
lesser of two evils thing.)
It's like the blank line between variable declarations and the rest of the code;
As much as I want to fit as much code on screen at once so you can see more of
it (which makes it more understandable if you ask me), paragraph breaks are also
a thing humans tend to need...
This is a good fix you've made, though:
-int update_password(char *filename, char* username, char* entry)
+int update_password(char *filename, char *username, char *entry)
(I think I wrote about it in code.html or design.html on the website, it's not
how C works: "char* a,b;" is not "char *a, *b;" it's "char *a, b;" and spacing
it the wrong way is MISLEADING.)
P.S. Part of the reason I haven't worked on this myself since your first patch
(other than trying to focus on toysh) is that my current version has had these
half-finished uncommitted changes in it for at least a month:
diff --git a/lib/password.c b/lib/password.c
index 3a39d9e6..9581610d 100644
--- a/lib/password.c
+++ b/lib/password.c
@@ -67,24 +66,26 @@ int read_password(char *buf, int buflen, char *mesg)
if ((ret = read(tty, buf+i, 1))<0 || (!ret&&!i) || *buf==4 || buf[i]==3) {
i = 0;
ret = 1;
+ } else if (!ret || buf[i] == '\n' || buf[i] == '\r') ret = 0;
+ else {
+ if (buf[i] == 8 || buf[i] == 127) i -= 2-!i;
- break;
- } else if (!ret || buf[i] == '\n' || buf[i] == '\r') {
- ret = 0;
+ continue;
+ }
+ buf[i] = 0;
- break;
- } else if (buf[i] == 8 || buf[i] == 127) i -= 2-!i;
+ break;
}
// Restore terminal/signal state, terminate string
sigaction(SIGINT, &oldsa, 0);
tcsetattr(0, TCSANOW, &oldtermio);
- buf[i] = 0;
xputc('\n');
return ret;
}
+// TODO this is completely broken
static char *get_nextcolon(char *line, int cnt)
{
while (cnt--) {
@@ -94,9 +95,8 @@ static char *get_nextcolon(char *line, int cnt)
return line;
}
-/*update_password is used by multiple utilities to update /etc/passwd,
- * /etc/shadow, /etc/group and /etc/gshadow files,
- * which are used as user, group databeses
+/* update colon-separated text password files, ala
+ * /etc/passwd, /etc/shadow, /etc/group and /etc/gshadow
* entry can be
* 1. encrypted password, when updating user password.
* 2. complete entry for user details, when creating new user
@@ -104,29 +104,23 @@ static char *get_nextcolon(char *line, int cnt)
* 4. complete entry for group details, when creating new group
* 5. entry = NULL, delete the named entry user/group
*/
-int update_password(char *filename, char* username, char* entry)
+int update_password(char *filename, char *username, char *entry)
{
- char *filenamesfx = NULL, *namesfx = NULL, *shadow = NULL,
- *sfx = NULL, *line = NULL;
- FILE *exfp, *newfp;
+ char *filenamesfx = 0, *namesfx = 0, *sfx, *line = 0;
+ FILE *exfp = 0, *newfp;
int ret = -1, found = 0, n;
struct flock lock;
size_t allocated_length;
- shadow = strstr(filename, "shadow");
- filenamesfx = xmprintf("%s+", filename);
- sfx = strchr(filenamesfx, '+');
-
- exfp = fopen(filename, "r+");
- if (!exfp) {
- perror_msg("Couldn't open file %s",filename);
+ sfx = strchr(filenamesfx = xmprintf("%s+", filename), '+');
+ if (!(exfp = fopen(filename, "r+"))) {
+ perror_msg("%s",filename);
goto free_storage;
}
*sfx = '-';
unlink(filenamesfx);
- ret = link(filename, filenamesfx);
- if (ret < 0) error_msg("can't create backup file");
+ if (0>link(filename, filenamesfx)) perror_msg("%s", filenamesfx);
*sfx = '+';
lock.l_type = F_WRLCK;
@@ -134,16 +128,11 @@ int update_password(char *filename, char* username, char*
entry)
lock.l_start = 0;
lock.l_len = 0;
- ret = fcntl(fileno(exfp), F_SETLK, &lock);
- if (ret < 0) perror_msg("Couldn't lock file %s",filename);
+ if (0>fcntl(fileno(exfp), F_SETLK, &lock)) perror_msg("%s", filename);
- lock.l_type = F_UNLCK; //unlocking at a later stage
-
- newfp = fopen(filenamesfx, "w+");
- if (!newfp) {
- error_msg("couldn't open file for writing");
+ if (!(newfp = fopen(filenamesfx, "w+"))) {
+ perror_msg("%s", filenamesfx);
ret = -1;
- fclose(exfp);
goto free_storage;
}
@@ -154,17 +143,16 @@ int update_password(char *filename, char* username, char*
entry)
if (strncmp(line, namesfx, strlen(namesfx)))
fprintf(newfp, "%s\n", line);
else if (entry) {
- char *current_ptr = NULL;
+ char *current_ptr = 0;
found = 1;
if (!strcmp(toys.which->name, "passwd")) {
fprintf(newfp, "%s%s:",namesfx, entry);
current_ptr = get_nextcolon(line, 2); //past passwd
- if (shadow) {
- fprintf(newfp, "%u:",(unsigned)(time(NULL))/(24*60*60));
- current_ptr = get_nextcolon(current_ptr, 1);
- fprintf(newfp, "%s\n",current_ptr);
- } else fprintf(newfp, "%s\n",current_ptr);
+ if (strstr(filename, "shadow"))
+ fprintf(newfp, "%llu:%s\n", (long long)time(0)/86400,
+ current_ptr = get_nextcolon(current_ptr, 1));
+ else fprintf(newfp, "%s\n", current_ptr);
} else if (!strcmp(toys.which->name, "groupadd") ||
!strcmp(toys.which->name, "addgroup") ||
!strcmp(toys.which->name, "delgroup") ||
@@ -179,8 +167,8 @@ int update_password(char *filename, char* username, char* entry)
free(line);
free(namesfx);
if (!found && entry) fprintf(newfp, "%s\n", entry);
+ lock.l_type = F_UNLCK;
fcntl(fileno(exfp), F_SETLK, &lock);
- fclose(exfp);
errno = 0;
fflush(newfp);
@@ -194,6 +182,8 @@ int update_password(char *filename, char* username, char* entry)
}
free_storage:
+ if (exfp) fclose(exfp);
free(filenamesfx);
+
return ret;
}
And THOSE changes depend on these unfinished/uncommitted changes:
diff --git a/lib/tty.c b/lib/tty.c
index b0d0c5d5..3e435fd8 100644
--- a/lib/tty.c
+++ b/lib/tty.c
@@ -74,9 +74,30 @@ void xsetspeed(struct termios *tio, int speed)
cfsetspeed(tio, i+1+4081*(i>15));
}
+// Reset terminal to known state, saving copy of old state if old != NULL.
+int tty_raw(int fd)
+{
+ struct termios tio;
+ int i = tcgetattr(fd, &tio);
+
+ // Fetch local copy of old terminfo, and copy struct contents to *old if set
+ if (i) return i;
+ toys.oldtty = tio;
+
+ // set terminal to know "raw" values. (Don't touch serial port cflags.)
+ tio.c_iflag = IUTF8|BRKINT; // Show UTF8, signal for serial BRK
+ tio.c_oflag = tio.c_cc[VTIME] = 0; // output unchanged, no O_NONBLOCK timeout
+ tio.c_lflag = ISIG|TOSTOP; // generate signals
+ tio.c_cc[VMIN] = 1; // read() returns when >= 1 char waiting
+
+ if (tcsetattr(toys.rawtty = fd, TCSAFLUSH, &tio)) return i;
+ sigatexit(tty_sigrestore);
+
+ return 0;
+}
// Reset terminal to known state, saving copy of old state if old != NULL.
-int set_terminal(int fd, int raw, int speed, struct termios *old)
+int set_terminal(int fd, int raw, int speed, struct termios *old) // TODO
{
struct termios termio;
int i = tcgetattr(fd, &termio);
@@ -123,7 +144,7 @@ int set_terminal(int fd, int raw, int speed, struct termios
*old)
return tcsetattr(fd, TCSAFLUSH, &termio);
}
-void xset_terminal(int fd, int raw, int speed, struct termios *old)
+void xset_terminal(int fd, int raw, int speed, struct termios *old) // TODO
{
if (-1 != set_terminal(fd, raw, speed, old)) return;
@@ -261,7 +282,25 @@ void tty_jump(int x, int y)
tty_esc(s);
}
-void tty_reset(void)
+// Emit tty exit sequence
+void tty_exit(void)
+{
+ // make cursor visible at bottom left corner, default color, clear last line
+ printf("\033[?25h\033[0;999H\033[0m\033[K");
+ fflush(0);
+}
+
+void tty_sigrestore(int i)
+{
+ tcsetattr(toys.rawtty, TCSAFLUSH, &toys.oldtty);
+ // make cursor visible, default color, bottom left of screen, clear last line
+ printf("\033[?25h\033[0m\033[0;999H\033[K");
+ fflush(0);
+
+ _exit(i ? 128+i : 0);
+}
+
+void tty_reset(void) // TODO
{
set_terminal(0, 0, 0, 0);
tty_esc("?25h");
@@ -272,7 +311,7 @@ void tty_reset(void)
}
// If you call set_terminal(), use sigatexit(tty_sigreset);
-void tty_sigreset(int i)
+void tty_sigreset(int i) // TODO
{
tty_reset();
_exit(i ? 128+i : 0);
@@ -284,7 +323,7 @@ void start_redraw(unsigned *width, unsigned *height)
if (!toys.signal) {
*width = 80;
*height = 25;
- set_terminal(0, 1, 0, 0);
+ set_terminal(0, 1, 0, 0); // TODO
sigatexit(tty_sigreset);
xsignal(SIGWINCH, generic_signal);
}
Which in turn was triggered by microcom.c setting the tty into a state where tab
characters were all zero length(?) because it didn't do a delta against the
existing tty and it turns out /dev/ttyS0 and /dev/pty123 have fundamentally
different settings which for some reason manifest in getting tab stops wrong
when used on a ttyACM0 serial port, so I have to redo all that plumbing to delta
against what was there before and store the old settings for the restore rather
than always writing "modern correct" settings on entry and exit like I thought I
could but turned out to be wrong about.
Now I have to back my changes out so this patch can capitalize comments, rename
variables, reorder variable declarations, and delete some blank lines, as a
dependency for the rest of your patch stack. Which is my fault and not yours, I
keep trailing off in the middle of stuff because I don't have enough time (doing
toybox is not my day job, I can't control when I get yanked to a more important
project) and then accumulate Technical Debt in my tree. (That's why I'm trying
to get the shell to a good stopping point before cutting a release, after
opening several cans of worms trying to get /usr/bin/ldd to run under it.
Leaving off in the middle means I've made MORE work for myself performing
archaeology to figure out where I left off and what I was trying to accomplish
and what subset of that I got done and what was left to do, rather than commited
code that passes its tests and doesn't have dangling threads to get tangled
while I'm away...)
Anyway, this is why it's taking me a while to get through it. Sorry.
Rob
More information about the Toybox
mailing list