Changeset View
Standalone View
usr.sbin/chown/chown.c
Show First 20 Lines • Show All 49 Lines • ▼ Show 20 Lines | |||||
#include <err.h> | #include <err.h> | ||||
#include <errno.h> | #include <errno.h> | ||||
#include <fcntl.h> | #include <fcntl.h> | ||||
#include <fts.h> | #include <fts.h> | ||||
#include <grp.h> | #include <grp.h> | ||||
#include <libgen.h> | #include <libgen.h> | ||||
#include <pwd.h> | #include <pwd.h> | ||||
#include <signal.h> | #include <signal.h> | ||||
#include <stddef.h> | |||||
#include <stdint.h> | #include <stdint.h> | ||||
#include <stdio.h> | #include <stdio.h> | ||||
#include <stdlib.h> | #include <stdlib.h> | ||||
#include <string.h> | #include <string.h> | ||||
#include <unistd.h> | #include <unistd.h> | ||||
static void a_gid(const char *); | static void a_gid(const char *); | ||||
static void a_uid(const char *); | static void a_uid(const char *); | ||||
static void chownerr(const char *); | static void chownerr(const char *); | ||||
static uid_t id(const char *, const char *); | static uid_t id(const char *, const char *); | ||||
darrick.freebsd_gmail.com: Possible whitespace error? | |||||
static void usage(void); | static void usage(void); | ||||
static void print_info(const FTSENT *, int); | static void print_info(const FTSENT *, int); | ||||
static uid_t uid; | static uid_t uid; | ||||
static gid_t gid; | static gid_t gid; | ||||
static int ischown; | static int ischown; | ||||
static const char *gname; | static const char *gname; | ||||
static volatile sig_atomic_t siginfo; | static volatile sig_atomic_t siginfo; | ||||
▲ Show 20 Lines • Show All 162 Lines • ▼ Show 20 Lines | |||||
{ | { | ||||
struct passwd *pw; | struct passwd *pw; | ||||
if (*s == '\0') /* Argument was "[:.]gid". */ | if (*s == '\0') /* Argument was "[:.]gid". */ | ||||
return; | return; | ||||
uid = ((pw = getpwnam(s)) != NULL) ? pw->pw_uid : id(s, "user"); | uid = ((pw = getpwnam(s)) != NULL) ? pw->pw_uid : id(s, "user"); | ||||
} | } | ||||
static uid_t | static uid_t | ||||
Not Done Inline ActionsWouldn't this result in a sign-extended return value if the uid == UID_MAX? id_t is signed. Given the static assert, it seems safe to keep the old return type. I don't think this makes a functional difference currently. markj: Wouldn't this result in a sign-extended return value if the uid == UID_MAX? id_t is signed. | |||||
Not Done Inline ActionsHm, I was not aware that id_t was signed. That seems unfortunate. It is probably to represent pid_t, but again — it is unclear why that is signed. I'm not sure about the integer promotion rules and sign extension. On LP64 platforms the unsigned long val should convert to id_t (== int64_t) without sign-extension. On 32-bit FreeBSD platforms, long is 32 bits. I would be somewhat surprised if conversion from one integer to another integer that can fully represent the range of the former caused sign extension, but I'll admit I am unfamiliar with the exact rules here. This seems to describe arithmetic; I don't know if it applies to what is essentially a cast:
(C18, §6.3.1.8, "Usual arithmetic conversions") §6.3.1.3 "Signed and unsigned integers" says "When a value with integer type is converted to another integer type other than _Bool , if the value Either way, I would be ok keeping the old return type here. cem: Hm, I was not aware that id_t was signed. That seems unfortunate. It is probably to represent… | |||||
Not Done Inline ActionsOops, I think you're right. I was thinking of the inverted case where we cast from a narrow signed type to a wider unsigned type. markj: Oops, I think you're right. I was thinking of the inverted case where we cast from a narrow… | |||||
Not Done Inline Actionspid_t is signed because -1 is used in some cases (e.g. killpg with -1 to kill the group you belong to) jhb: pid_t is signed because -1 is used in some cases (e.g. killpg with -1 to kill the group you… | |||||
Not Done Inline ActionsSure — but it could just as easily use UINT_MAX as sentinel value instead of -1 :-). cem: Sure — but it could just as easily use UINT_MAX as sentinel value instead of -1 :-). | |||||
Not Done Inline ActionsIt could have, but that ship has kind of sailed (killpg is part of POSIX, etc.) jhb: It could have, but that ship has kind of sailed (killpg is part of POSIX, etc.) | |||||
Not Done Inline ActionsSure, understood we can't change it now. Curiously, Linux' killpg takes int, not pid_t, and our killpg does not document a pgrp=-1 behavior. I am curious what POSIX specifies. POSIX says:
Oh, I see:
So it's that kill() is required to take a negative first argument, and the type of that argument must be pid_t. Got it. And the value can be any negative process id, not just 1, and of course progress group ids are just pid's. Thanks :-). cem: Sure, understood we can't change it now.
Curiously, Linux' killpg takes int, not pid_t, and… | |||||
id(const char *name, const char *type) | id(const char *name, const char *type) | ||||
{ | { | ||||
uid_t val; | unsigned long val; | ||||
char *ep; | char *ep; | ||||
/* | |||||
* XXX | |||||
* We know that uid_t's and gid_t's are unsigned longs. | |||||
*/ | |||||
errno = 0; | errno = 0; | ||||
val = strtoul(name, &ep, 10); | val = strtoul(name, &ep, 10); | ||||
if (errno || *ep != '\0') | _Static_assert(UID_MAX >= GID_MAX, "UID MAX less than GID MAX"); | ||||
Not Done Inline ActionsThis could be relaxed to >=, given a return type of either uid_t or id_t (it may be allowed for id_t to be larger than uid_t, for example). cem: This could be relaxed to `>=`, given a return type of either `uid_t` or `id_t` (it may be… | |||||
Not Done Inline ActionsI am not sure why >= and not ==. UID_MAX is for uid_t not for id_t. Why not just use sizeof()s ? kib: I am not sure why >= and not ==. UID_MAX is for uid_t not for id_t.
Why not just use sizeof()s… | |||||
Not Done Inline ActionsI could have been more clear. When the return type is either uid_t or id_t, this code is correct even if gid_t (or just GID_MAX) is smaller than uid_t (or UID_MAX). If the opposite is true (gid_t larger than uid_t), then this code is incorrect when the return type is kept as uid_t. (id_t return type always works, since it is guaranteed to be able to hold a uid, gid, or pid value.) cem: I could have been more clear.
When the return type is either `uid_t` or `id_t`, this code is… | |||||
if (errno || *ep != '\0' || val > UID_MAX) | |||||
Not Done Inline ActionsStyle nit: unneeded parens. markj: Style nit: unneeded parens. | |||||
kibUnsubmitted Not Done Inline Actionsif (errno != 0 || .... kib: if (errno != 0 || .... | |||||
cemUnsubmitted Not Done Inline Actions(Not introduced in this diff -- only the extra OR case was appended. But it can be fixed at commit.) cem: (Not introduced in this diff -- only the extra OR case was appended. But it can be fixed at… | |||||
errx(1, "%s: illegal %s name", name, type); | errx(1, "%s: illegal %s name", name, type); | ||||
delphijUnsubmitted Not Done Inline ActionsUnrelated to the change itself, but shouldn't the 'name"' be replaced by 'id"'? delphij: Unrelated to the change itself, but shouldn't the 'name"' be replaced by 'id"'? | |||||
return (val); | return (val); | ||||
} | } | ||||
static void | static void | ||||
chownerr(const char *file) | chownerr(const char *file) | ||||
{ | { | ||||
static uid_t euid = -1; | static uid_t euid = -1; | ||||
static int ngroups = -1; | static int ngroups = -1; | ||||
▲ Show 20 Lines • Show All 64 Lines • Show Last 20 Lines |
Possible whitespace error?