From: Otto Moerbeek <[email protected]>
Date: Fri, 16 Aug 2019 13:56:37 +0200
> 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.
As you say, the "!" check is not a incorrect.
Thank you.
--
ASOU Masato
> -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
>>
