Skip to content

Use read_all/pread_all where we expect full buffer to be read #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: crac
Choose a base branch
from
Open

Use read_all/pread_all where we expect full buffer to be read #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b6f698
Use read_all/pread_all where we expect full buffer to be read
rvansa Oct 6, 2023
7c06d3e
Return -1 on error in restore_content()
rvansa Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion criu/apparmor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ static int collect_profile(char *path, int offset, char *dir, AaNamespace *ns)
if (!cur->blob.data)
goto close;

n = read(fd, cur->blob.data, sb.st_size);
n = read_all(fd, cur->blob.data, sb.st_size);
if (n < 0) {
pr_perror("failed to read %s", path);
goto close;
Expand Down
2 changes: 1 addition & 1 deletion criu/cr-dump.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ static int get_task_auxv(pid_t pid, MmEntry *mm)
if (fd < 0)
return -1;

ret = read(fd, mm_saved_auxv, sizeof(mm_saved_auxv));
ret = read_all(fd, mm_saved_auxv, sizeof(mm_saved_auxv));
if (ret < 0) {
ret = -1;
pr_perror("Error reading %d's auxv", pid);
Expand Down
2 changes: 1 addition & 1 deletion criu/files-reg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1576,7 +1576,7 @@ static int get_build_id(const int fd, const struct stat *fd_status, unsigned cha
size_t mapped_size;
int ret = -1;

if (read(fd, buf, SELFMAG + 1) != SELFMAG + 1)
if (read_all(fd, buf, SELFMAG + 1) != SELFMAG + 1)
return -1;

/*
Expand Down
1 change: 1 addition & 0 deletions criu/include/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@ extern char *get_legacy_iptables_bin(bool ipv6);

extern ssize_t read_all(int fd, void *buf, size_t size);
extern ssize_t write_all(int fd, const void *buf, size_t size);
extern ssize_t pread_all(int fd, void *buf, size_t size, off_t offset);

#define cleanup_free __attribute__((cleanup(cleanup_freep)))
static inline void cleanup_freep(void *p)
Expand Down
20 changes: 7 additions & 13 deletions criu/ipc_ns.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,26 +790,20 @@ static int prepare_ipc_msg(int pid)
static int restore_content(void *data, struct cr_img *img, const IpcShmEntry *shm)
{
int ifd;
ssize_t size, off;
ssize_t size, ret;

ifd = img_raw_fd(img);
if (ifd < 0) {
pr_err("Failed getting raw image fd\n");
return -1;
}
size = round_up(shm->size, sizeof(u32));
off = 0;
do {
ssize_t ret;

ret = read(ifd, data + off, size - off);
if (ret <= 0) {
pr_perror("Failed to write IPC shared memory data");
return (int)ret;
}

off += ret;
} while (off < size);
size = round_up(shm->size, sizeof(u32));
ret = read_all(ifd, data, size);
if (ret != size) {
pr_perror("Failed to write IPC shared memory data");
return -1;
}

return 0;
}
Expand Down
6 changes: 3 additions & 3 deletions criu/kerndat.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ static int check_pagemap(void)
}

/* Get the PFN of some present page. Stack is here, so try it :) */
ret = pread(fd, &pfn, sizeof(pfn), (((unsigned long)&ret) / page_size()) * sizeof(pfn));
ret = pread_all(fd, &pfn, sizeof(pfn), (((unsigned long)&ret) / page_size()) * sizeof(pfn));
if (ret != sizeof(pfn)) {
pr_perror("Can't read pagemap");
return -1;
Expand Down Expand Up @@ -1090,7 +1090,7 @@ static int kerndat_try_load_cache(void)
return 1;
}

ret = read(fd, &kdat, sizeof(kdat));
ret = read_all(fd, &kdat, sizeof(kdat));
if (ret < 0) {
pr_perror("Can't read kdat cache");
close(fd);
Expand Down Expand Up @@ -1361,7 +1361,7 @@ static int kerndat_has_pidfd_getfd(void)
goto close_all;
}

if (read(fds[1], &val_b, sizeof(val_b)) != sizeof(val_b)) {
if (read_all(fds[1], &val_b, sizeof(val_b)) != sizeof(val_b)) {
pr_perror("Can't read from socket");
ret = -1;
goto close_all;
Expand Down
2 changes: 1 addition & 1 deletion criu/pagemap-cache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ static int pmc_fill_cache(pmc_t *pmc, const struct vma_area *vma)
BUG_ON(pmc->map_len < size_map);
BUG_ON(pmc->fd < 0);

if (pread(pmc->fd, pmc->map, size_map, PAGEMAP_PFN_OFF(pmc->start)) != size_map) {
if (pread_all(pmc->fd, pmc->map, size_map, PAGEMAP_PFN_OFF(pmc->start)) != size_map) {
pmc_zap(pmc);
pr_perror("Can't read %d's pagemap file", pmc->pid);
return -1;
Expand Down
34 changes: 11 additions & 23 deletions criu/pagemap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,6 @@ static int read_local_page(struct page_read *pr, unsigned long vaddr, unsigned l
{
int fd;
ssize_t ret;
size_t curr = 0;

fd = img_raw_fd(pr->pi);
if (fd < 0) {
Expand All @@ -250,15 +249,10 @@ static int read_local_page(struct page_read *pr, unsigned long vaddr, unsigned l
return -1;

pr_debug("\tpr%lu-%u Read page from self %lx/%" PRIx64 "\n", pr->img_id, pr->id, pr->cvaddr, pr->pi_off);
while (1) {
ret = pread(fd, buf + curr, len - curr, pr->pi_off + curr);
if (ret < 1) {
pr_perror("Can't read mapping page %zd", ret);
return -1;
}
curr += ret;
if (curr == len)
break;
ret = pread_all(fd, buf, len, pr->pi_off);
if (ret != len) {
pr_perror("Can't read mapping page %zd", ret);
return -1;
}

if (opts.auto_dedup) {
Expand Down Expand Up @@ -407,7 +401,6 @@ static int maybe_read_page_img_streamer(struct page_read *pr, unsigned long vadd
unsigned long len = nr * PAGE_SIZE;
int fd;
int ret;
size_t curr = 0;

fd = img_raw_fd(pr->pi);
if (fd < 0) {
Expand All @@ -420,18 +413,13 @@ static int maybe_read_page_img_streamer(struct page_read *pr, unsigned long vadd
/* We can't seek. The requested address better match */
BUG_ON(pr->cvaddr != vaddr);

while (1) {
ret = read(fd, buf + curr, len - curr);
if (ret == 0) {
pr_err("Reached EOF unexpectedly while reading page from image\n");
return -1;
} else if (ret < 0) {
pr_perror("Can't read mapping page %d", ret);
return -1;
}
curr += ret;
if (curr == len)
break;
ret = read_all(fd, buf, len);
if (ret < len) {
pr_err("Reached EOF unexpectedly while reading page from image\n");
return -1;
} else if (ret < 0) {
pr_perror("Can't read mapping page %d", ret);
return -1;
}

if (opts.auto_dedup)
Expand Down
28 changes: 27 additions & 1 deletion criu/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,7 +840,7 @@ int vaddr_to_pfn(int fd, unsigned long vaddr, u64 *pfn)
}

off = (vaddr / page_size()) * sizeof(u64);
ret = pread(fd, pfn, sizeof(*pfn), off);
ret = pread_all(fd, pfn, sizeof(*pfn), off);
if (ret != sizeof(*pfn)) {
pr_perror("Can't read pme for pid %d", getpid());
ret = -1;
Expand Down Expand Up @@ -1686,6 +1686,32 @@ ssize_t write_all(int fd, const void *buf, size_t size)
return n;
}

ssize_t pread_all(int fd, void *buf, size_t size, off_t offset)
{
ssize_t n = 0;
while (size > 0) {
ssize_t ret = pread(fd, buf, size, offset);
if (ret == -1) {
if (errno == EINTR)
continue;
/*
* The caller should use standard read() for
* non-blocking I/O.
*/
if (errno == EAGAIN || errno == EWOULDBLOCK)
errno = EINVAL;
Comment on lines +1701 to +1702
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This disturbs me, impact is not clear. Later we can process the code. Let's keep errno intact?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually a copy-paste of the read_all method above (just read -> pread). I don't see a problem in writing to errno?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AntonKozlov
I agree with @rvansa - we could keep this consistent with other existing functions.
BTW I'd prefer using curly braces for one-line if's, but just a note.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep the @rvansa code. It is a copy-paste of read_all - while I would prefer its removal it would be a needless fork from upstream CRIU and it is not really required. If something then it could be rather an upstream CRIU contribution.
@wkia: CRIU does not use curly braces for one-line ifs: https://github.com/CRaC/criu/blob/crac/CONTRIBUTING.md#edit-the-source-code

return ret;
}
if (ret == 0)
break;
n += ret;
buf = (char *)buf + ret;
size -= ret;
offset += ret;
}
return n;
}

static int remove_one(const char *fpath, const struct stat *sb, int tflag, struct FTW *ftwbuf)
{
int ret;
Expand Down