On Fri, Aug 16, 2019 at 05:44:35PM +0900, Masato Asou wrote:
> Incorrect checking the return value of malloc and system calls in
> /bin/cp.
>
> ok?
Altough I indeed prefer to check against NULL these are not incorrect,
just bad idiom.
-Otto
>
> Index: utils.c
> ===================================================================
> RCS file: /cvs/src/bin/cp/utils.c,v
> retrieving revision 1.48
> diff -u -p -U10 -r1.48 utils.c
> --- utils.c 28 Jun 2019 13:34:58 -0000 1.48
> +++ utils.c 16 Aug 2019 08:19:43 -0000
> @@ -53,28 +53,28 @@ int
> copy_file(FTSENT *entp, int exists)
> {
> static char *buf;
> static char *zeroes;
> struct stat to_stat, *fs;
> int from_fd, rcount, rval, to_fd, wcount;
> #ifdef VM_AND_BUFFER_CACHE_SYNCHRONIZED
> char *p;
> #endif
>
> - if (!buf) {
> + if (buf == NULL) {
> buf = malloc(MAXBSIZE);
> - if (!buf)
> + if (buf == NULL)
> err(1, "malloc");
> }
> - if (!zeroes) {
> + if (zeroes == NULL) {
> zeroes = calloc(1, MAXBSIZE);
> - if (!zeroes)
> + if (zeroes == NULL)
> err(1, "calloc");
> }
>
> if ((from_fd = open(entp->fts_path, O_RDONLY, 0)) == -1) {
> warn("%s", entp->fts_path);
> return (1);
> }
>
> fs = entp->fts_statp;
>
> @@ -132,21 +132,21 @@ copy_file(FTSENT *entp, int exists)
> if (munmap(p, fs->st_size) == -1) {
> warn("%s", entp->fts_path);
> rval = 1;
> }
> }
> } else
> #endif
> {
> int skipholes = 0;
> struct stat tosb;
> - if (!fstat(to_fd, &tosb) && S_ISREG(tosb.st_mode))
> + if (fstat(to_fd, &tosb) == 0 && S_ISREG(tosb.st_mode))
> skipholes = 1;
> while ((rcount = read(from_fd, buf, MAXBSIZE)) > 0) {
> if (skipholes && memcmp(buf, zeroes, rcount) == 0)
> wcount = lseek(to_fd, rcount, SEEK_CUR) == -1 ?
> -1 : rcount;
> else
> wcount = write(to_fd, buf, rcount);
> if (rcount != wcount || wcount == -1) {
> warn("%s", to.p_path);
> rval = 1;
> break;
> @@ -169,87 +169,87 @@ copy_file(FTSENT *entp, int exists)
> if (pflag && setfile(fs, to_fd))
> rval = 1;
> /*
> * If the source was setuid or setgid, lose the bits unless the
> * copy is owned by the same user and group.
> */
> #define RETAINBITS \
> (S_ISUID | S_ISGID | S_ISVTX | S_IRWXU | S_IRWXG | S_IRWXO)
> if (!pflag && !exists &&
> fs->st_mode & (S_ISUID | S_ISGID) && fs->st_uid == myuid) {
> - if (fstat(to_fd, &to_stat)) {
> + if (fstat(to_fd, &to_stat) == -1) {
> warn("%s", to.p_path);
> rval = 1;
> } else if (fs->st_gid == to_stat.st_gid &&
> - fchmod(to_fd, fs->st_mode & RETAINBITS & ~myumask)) {
> + fchmod(to_fd, fs->st_mode & RETAINBITS & ~myumask) == -1) {
> warn("%s", to.p_path);
> rval = 1;
> }
> }
> (void)close(from_fd);
> - if (close(to_fd)) {
> + if (close(to_fd) == -1) {
> warn("%s", to.p_path);
> rval = 1;
> }
> return (rval);
> }
>
> int
> copy_link(FTSENT *p, int exists)
> {
> int len;
> char name[PATH_MAX];
>
> if (exists && !copy_overwrite())
> return (2);
> if ((len = readlink(p->fts_path, name, sizeof(name)-1)) == -1) {
> warn("readlink: %s", p->fts_path);
> return (1);
> }
> name[len] = '\0';
> - if (exists && unlink(to.p_path)) {
> + if (exists && unlink(to.p_path) == -1) {
> warn("unlink: %s", to.p_path);
> return (1);
> }
> - if (symlink(name, to.p_path)) {
> + if (symlink(name, to.p_path) == -1) {
> warn("symlink: %s", name);
> return (1);
> }
> return (pflag ? setfile(p->fts_statp, -1) : 0);
> }
>
> int
> copy_fifo(struct stat *from_stat, int exists)
> {
> if (exists && !copy_overwrite())
> return (2);
> - if (exists && unlink(to.p_path)) {
> + if (exists && unlink(to.p_path) == -1) {
> warn("unlink: %s", to.p_path);
> return (1);
> }
> - if (mkfifo(to.p_path, from_stat->st_mode)) {
> + if (mkfifo(to.p_path, from_stat->st_mode) == -1) {
> warn("mkfifo: %s", to.p_path);
> return (1);
> }
> return (pflag ? setfile(from_stat, -1) : 0);
> }
>
> int
> copy_special(struct stat *from_stat, int exists)
> {
> if (exists && !copy_overwrite())
> return (2);
> - if (exists && unlink(to.p_path)) {
> + if (exists && unlink(to.p_path) == -1) {
> warn("unlink: %s", to.p_path);
> return (1);
> }
> - if (mknod(to.p_path, from_stat->st_mode, from_stat->st_rdev)) {
> + if (mknod(to.p_path, from_stat->st_mode, from_stat->st_rdev) == -1) {
> warn("mknod: %s", to.p_path);
> return (1);
> }
> return (pflag ? setfile(from_stat, -1) : 0);
> }
>
> /*
> * If the file exists and we're interactive, verify with the user.
> */
> int
> @@ -272,55 +272,57 @@ int
> setfile(struct stat *fs, int fd)
> {
> struct timespec ts[2];
> int rval;
>
> rval = 0;
> fs->st_mode &= S_ISTXT | S_ISUID | S_ISGID | S_IRWXU | S_IRWXG |
> S_IRWXO;
>
> ts[0] = fs->st_atim;
> ts[1] = fs->st_mtim;
> - if (fd >= 0 ? futimens(fd, ts) :
> - utimensat(AT_FDCWD, to.p_path, ts, AT_SYMLINK_NOFOLLOW)) {
> + if (fd >= 0 ? futimens(fd, ts) == -1 :
> + utimensat(AT_FDCWD, to.p_path, ts, AT_SYMLINK_NOFOLLOW) == -1) {
> warn("update times: %s", to.p_path);
> rval = 1;
> }
> /*
> * Changing the ownership probably won't succeed, unless we're root
> * or POSIX_CHOWN_RESTRICTED is not set. Set uid/gid before setting
> * the mode; current BSD behavior is to remove all setuid bits on
> * chown. If chown fails, lose setuid/setgid bits.
> */
> - if (fd >= 0 ? fchown(fd, fs->st_uid, fs->st_gid) :
> - lchown(to.p_path, fs->st_uid, fs->st_gid)) {
> + if (fd >= 0 ? fchown(fd, fs->st_uid, fs->st_gid) == -1 :
> + lchown(to.p_path, fs->st_uid, fs->st_gid) == -1) {
> if (errno != EPERM) {
> warn("chown: %s", to.p_path);
> rval = 1;
> }
> fs->st_mode &= ~(S_ISTXT | S_ISUID | S_ISGID);
> }
> - if (fd >= 0 ? fchmod(fd, fs->st_mode) :
> - fchmodat(AT_FDCWD, to.p_path, fs->st_mode, AT_SYMLINK_NOFOLLOW)) {
> + if (fd >= 0 ? fchmod(fd, fs->st_mode) == -1 :
> + fchmodat(AT_FDCWD, to.p_path, fs->st_mode, AT_SYMLINK_NOFOLLOW)
> + == -1) {
> warn("chmod: %s", to.p_path);
> rval = 1;
> }
>
> /*
> * XXX
> * NFS doesn't support chflags; ignore errors unless there's reason
> * to believe we're losing bits. (Note, this still won't be right
> * if the server supports flags and we were trying to *remove* flags
> * on a file that we copied, i.e., that we didn't create.)
> */
> errno = 0;
> - if (fd >= 0 ? fchflags(fd, fs->st_flags) :
> - chflagsat(AT_FDCWD, to.p_path, fs->st_flags, AT_SYMLINK_NOFOLLOW))
> + if (fd >= 0 ? fchflags(fd, fs->st_flags) == -1 :
> + chflagsat(AT_FDCWD, to.p_path, fs->st_flags, AT_SYMLINK_NOFOLLOW)
> + == -1)
> if (errno != EOPNOTSUPP || fs->st_flags != 0) {
> warn("chflags: %s", to.p_path);
> rval = 1;
> }
> return (rval);
> }
>
>
> void
> usage(void)
>
> --
> ASOU Masato
>
