[PATCH 0/2] Implement fgetspent

[PATCH 0/2] Implement fgetspent

[Michael Forney]

Hi Rich,

It turns out that one more function, fgetspent, is needed for the common
shadow+pam_unix setup used in many desktop systems. Otherwise, when
changing your password, pam_unix will attempt to enumerate through the
shadow entries with fgetspent, rewriting them as it goes (and replacing
the entry to be updated). This leaves you with a shadow file containing
only the updated entry.

In the first patch, I moved spent parsing to an internal function,
__parsespent. I opted to use __parsespent instead of __getspent_a
(similar to the passwd and group functions) for several reasons:

    - To minimize the changes necessary to getspnam_r
    - To avoid the extra memcpy as in getpw_r
    - It seemed like a more self-contained function (which didn't rely
      on the source of the entry).
    - It would make it easier to implement sgetspent if we ever wanted
      that (though, so far, I haven't found anything that requires this)

However, if this is not desired, I can send a new patch which uses a
function __getspent_a, similar to __get{pw,gr}ent_a.

Michael Forney (2):
  shadow: Move spent parsing to internal function
  shadow: Implement fgetspent

 src/passwd/fgetspent.c  | 11 ++++++++++-
 src/passwd/getspnam_r.c | 34 +---------------------------------
 src/passwd/parsespent.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 src/passwd/pwf.h        |  2 +-
 4 files changed, 54 insertions(+), 35 deletions(-)
 create mode 100644 src/passwd/parsespent.c

-- 
1.8.4.2

[PATCH 1/2] shadow: Move spent parsing to internal function

[Michael Forney]

---
 src/passwd/getspnam_r.c | 34 +---------------------------------
 src/passwd/parsespent.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 src/passwd/pwf.h        |  2 +-
 3 files changed, 44 insertions(+), 34 deletions(-)
 create mode 100644 src/passwd/parsespent.c

diff --git a/src/passwd/getspnam_r.c b/src/passwd/getspnam_r.c
index f4d7b35..59a84b3 100644
--- a/src/passwd/getspnam_r.c
+++ b/src/passwd/getspnam_r.c
@@ -12,11 +12,6 @@
  * file. It also avoids any allocation to prevent memory-exhaustion
  * attacks via huge TCB shadow files. */
 
-static long xatol(const char *s)
-{
-	return isdigit(*s) ? atol(s) : -1;
-}
-
 static void cleanup(void *p)
 {
 	fclose(p);
@@ -29,7 +24,6 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct
 	int rv = 0;
 	int fd;
 	size_t k, l = strlen(name);
-	char *s;
 	int skip = 0;
 	int cs;
 
@@ -71,34 +65,8 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct
 			rv = ERANGE;
 			break;
 		}
-		buf[k-1] = 0;
-
-		s = buf;
-		sp->sp_namp = s;
-		if (!(s = strchr(s, ':'))) continue;
-
-		*s++ = 0; sp->sp_pwdp = s;
-		if (!(s = strchr(s, ':'))) continue;
-
-		*s++ = 0; sp->sp_lstchg = xatol(s);
-		if (!(s = strchr(s, ':'))) continue;
-
-		*s++ = 0; sp->sp_min = xatol(s);
-		if (!(s = strchr(s, ':'))) continue;
-
-		*s++ = 0; sp->sp_max = xatol(s);
-		if (!(s = strchr(s, ':'))) continue;
-
-		*s++ = 0; sp->sp_warn = xatol(s);
-		if (!(s = strchr(s, ':'))) continue;
-
-		*s++ = 0; sp->sp_inact = xatol(s);
-		if (!(s = strchr(s, ':'))) continue;
-
-		*s++ = 0; sp->sp_expire = xatol(s);
-		if (!(s = strchr(s, ':'))) continue;
 
-		*s++ = 0; sp->sp_flag = xatol(s);
+		if (__parsespent(buf, sp) < 0) continue;
 		*res = sp;
 		break;
 	}
diff --git a/src/passwd/parsespent.c b/src/passwd/parsespent.c
new file mode 100644
index 0000000..d781387
--- /dev/null
+++ b/src/passwd/parsespent.c
@@ -0,0 +1,42 @@
+#include "pwf.h"
+
+static long xatol(char **s)
+{
+	long x;
+	if (**s == ':' || **s == '\n') return -1;
+	for (x=0; **s-'0'<10U; ++*s) x=10*x+(**s-'0');
+	return x;
+}
+
+int __parsespent(char *s, struct spwd *sp)
+{
+	sp->sp_namp = s;
+	if (!(s = strchr(s, ':'))) return -1;
+	*s = 0;
+
+	sp->sp_pwdp = ++s;
+	if (!(s = strchr(s, ':'))) return -1;
+	*s = 0;
+
+	s++; sp->sp_lstchg = xatol(&s);
+	if (*s != ':') return -1;
+
+	s++; sp->sp_min = xatol(&s);
+	if (*s != ':') return -1;
+
+	s++; sp->sp_max = xatol(&s);
+	if (*s != ':') return -1;
+
+	s++; sp->sp_warn = xatol(&s);
+	if (*s != ':') return -1;
+
+	s++; sp->sp_inact = xatol(&s);
+	if (*s != ':') return -1;
+
+	s++; sp->sp_expire = xatol(&s);
+	if (*s != ':') return -1;
+
+	s++; sp->sp_flag = xatol(&s);
+	if (*s != '\n') return -1;
+	return 0;
+}
diff --git a/src/passwd/pwf.h b/src/passwd/pwf.h
index 0a76ef8..2d813ad 100644
--- a/src/passwd/pwf.h
+++ b/src/passwd/pwf.h
@@ -9,5 +9,5 @@
 #include "libc.h"
 
 struct passwd *__getpwent_a(FILE *f, struct passwd *pw, char **line, size_t *size);
-struct spwd *__getspent_a(FILE *f, struct spwd *sp, char **line, size_t *size);
 struct group *__getgrent_a(FILE *f, struct group *gr, char **line, size_t *size, char ***mem, size_t *nmem);
+int __parsespent(char *s, struct spwd *sp);
-- 
1.8.4.2

[PATCH v2 1/2] shadow: Move spent parsing to internal function

[Michael Forney]

---
 src/passwd/getspnam_r.c | 69 ++++++++++++++++++++++++++++---------------------
 src/passwd/pwf.h        |  2 +-
 2 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/src/passwd/getspnam_r.c b/src/passwd/getspnam_r.c
index f4d7b35..15f8c87 100644
--- a/src/passwd/getspnam_r.c
+++ b/src/passwd/getspnam_r.c
@@ -12,9 +12,45 @@
  * file. It also avoids any allocation to prevent memory-exhaustion
  * attacks via huge TCB shadow files. */
 
-static long xatol(const char *s)
+static long xatol(char **s)
 {
-	return isdigit(*s) ? atol(s) : -1;
+	long x;
+	if (**s == ':' || **s == '\n') return -1;
+	for (x=0; **s-'0'<10U; ++*s) x=10*x+(**s-'0');
+	return x;
+}
+
+int __parsespent(char *s, struct spwd *sp)
+{
+	sp->sp_namp = s;
+	if (!(s = strchr(s, ':'))) return -1;
+	*s = 0;
+
+	sp->sp_pwdp = ++s;
+	if (!(s = strchr(s, ':'))) return -1;
+	*s = 0;
+
+	s++; sp->sp_lstchg = xatol(&s);
+	if (*s != ':') return -1;
+
+	s++; sp->sp_min = xatol(&s);
+	if (*s != ':') return -1;
+
+	s++; sp->sp_max = xatol(&s);
+	if (*s != ':') return -1;
+
+	s++; sp->sp_warn = xatol(&s);
+	if (*s != ':') return -1;
+
+	s++; sp->sp_inact = xatol(&s);
+	if (*s != ':') return -1;
+
+	s++; sp->sp_expire = xatol(&s);
+	if (*s != ':') return -1;
+
+	s++; sp->sp_flag = xatol(&s);
+	if (*s != '\n') return -1;
+	return 0;
 }
 
 static void cleanup(void *p)
@@ -29,7 +65,6 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct
 	int rv = 0;
 	int fd;
 	size_t k, l = strlen(name);
-	char *s;
 	int skip = 0;
 	int cs;
 
@@ -71,34 +106,8 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct
 			rv = ERANGE;
 			break;
 		}
-		buf[k-1] = 0;
-
-		s = buf;
-		sp->sp_namp = s;
-		if (!(s = strchr(s, ':'))) continue;
-
-		*s++ = 0; sp->sp_pwdp = s;
-		if (!(s = strchr(s, ':'))) continue;
-
-		*s++ = 0; sp->sp_lstchg = xatol(s);
-		if (!(s = strchr(s, ':'))) continue;
-
-		*s++ = 0; sp->sp_min = xatol(s);
-		if (!(s = strchr(s, ':'))) continue;
-
-		*s++ = 0; sp->sp_max = xatol(s);
-		if (!(s = strchr(s, ':'))) continue;
-
-		*s++ = 0; sp->sp_warn = xatol(s);
-		if (!(s = strchr(s, ':'))) continue;
-
-		*s++ = 0; sp->sp_inact = xatol(s);
-		if (!(s = strchr(s, ':'))) continue;
-
-		*s++ = 0; sp->sp_expire = xatol(s);
-		if (!(s = strchr(s, ':'))) continue;
 
-		*s++ = 0; sp->sp_flag = xatol(s);
+		if (__parsespent(buf, sp) < 0) continue;
 		*res = sp;
 		break;
 	}
diff --git a/src/passwd/pwf.h b/src/passwd/pwf.h
index 0a76ef8..2d813ad 100644
--- a/src/passwd/pwf.h
+++ b/src/passwd/pwf.h
@@ -9,5 +9,5 @@
 #include "libc.h"
 
 struct passwd *__getpwent_a(FILE *f, struct passwd *pw, char **line, size_t *size);
-struct spwd *__getspent_a(FILE *f, struct spwd *sp, char **line, size_t *size);
 struct group *__getgrent_a(FILE *f, struct group *gr, char **line, size_t *size, char ***mem, size_t *nmem);
+int __parsespent(char *s, struct spwd *sp);
-- 
1.8.4.2

[PATCH 2/2] shadow: Implement fgetspent

[Michael Forney]

---
 src/passwd/fgetspent.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/passwd/fgetspent.c b/src/passwd/fgetspent.c
index 3dda784..47473bd 100644
--- a/src/passwd/fgetspent.c
+++ b/src/passwd/fgetspent.c
@@ -1,6 +1,15 @@
 #include "pwf.h"
+#include <pthread.h>
 
 struct spwd *fgetspent(FILE *f)
 {
-	return 0;
+	static char *line;
+	static struct spwd sp;
+	size_t size = 0;
+	struct spwd *res = 0;
+	int cs;
+	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
+	if (getline(&line, &size, f) >= 0 && __parsespent(line, &sp) >= 0) res = &sp;
+	pthread_setcancelstate(cs, 0);
+	return res;
 }
-- 
1.8.4.2

Re: [PATCH 0/2] Implement fgetspent

[Rich Felker]

On Wed, Nov 20, 2013 at 07:13:39PM -0800, Michael Forney wrote:
> Hi Rich,
> 
> It turns out that one more function, fgetspent, is needed for the common
> shadow+pam_unix setup used in many desktop systems. Otherwise, when
> changing your password, pam_unix will attempt to enumerate through the
> shadow entries with fgetspent, rewriting them as it goes (and replacing
> the entry to be updated). This leaves you with a shadow file containing
> only the updated entry.
> 
> In the first patch, I moved spent parsing to an internal function,
> __parsespent. I opted to use __parsespent instead of __getspent_a
> (similar to the passwd and group functions) for several reasons:
> 
>     - To minimize the changes necessary to getspnam_r
>     - To avoid the extra memcpy as in getpw_r
>     - It seemed like a more self-contained function (which didn't rely
>       on the source of the entry).
>     - It would make it easier to implement sgetspent if we ever wanted
>       that (though, so far, I haven't found anything that requires this)
> 
> However, if this is not desired, I can send a new patch which uses a
> function __getspent_a, similar to __get{pw,gr}ent_a.

Overall I like this, but I don't see any advantage to putting
__parsespent in a separate file. On the surface it appears to reduce
dependencies for static linking, but any real-world use of fgetspent
would already depend on most of the functions getspnam_r depends on.
On the other hand, each additional file contributes to build time,
static library size (from elf header overhead), and potentially
hitting the command line limits on the ar/ld command lines. If you
don't have any objections, how about leaving the parsing code in
getspnam_r.c and only adding a new file for fgetspent?

Rich