From: Omar Polo Subject: Re: gotwebd: openat(2) and less params for scraping repo info To: Omar Polo Cc: gameoftrees@openbsd.org Date: Tue, 22 Nov 2022 12:07:49 +0100 On 2022/11/15 15:56:29 +0100, Omar Polo wrote: > On 2022/11/15 09:56:23 +0100, Omar Polo wrote: > > Here's a reiteration of the previous diff. Since i was already using > > openat(2) in gotweb_get_repo_{description,clone_url} i thought i could > > avoid the FILE handling at all: all we need to do is seek a bit to see > > how big the file is and read from it. It also changes len to be off_t > > since now it's using lseek instead of fseek. > > > > There is also another small change bundled. > > gotweb_get_repo_description and clone_url differs in how they handle > > the "len == 0" case when reading the file. One sets the return to "", > > the other one leaves NULL. I'm normalizing the behaviour to return > > NULL in both functions. > > clarified this point with Tracey, the intent is to never end up > passing NULL to fcgi_printf but since we already fall back to "" if > the description or the clone url is NULL I think it's fine. Tested > with both file absent and/or empty and gotwebd doesn't print "(null)". > > On a second thought we could drop a few more lines and don't special > casing empty files. We would end up allocating one byte, read zero > into it and return. Don't know if it's preferable, I'm being seduced > by having more - in the diff. ping. I've extracted one bit and sent as a separate mail. ----------------------------------------------- commit b7c8ea3da1b8cca57cb1d6c726ed9b2590ee5ec6 (w) from: Omar Polo date: Tue Nov 22 11:00:49 2022 UTC gotwebd: rework gotweb_get_repo_{description,cloneurl} - use openat(2) since we've already opened the containing dir - simplify reading the files diff 39afa5819b25cb2a5a8411de6f16d1e5667bb2c7 b7c8ea3da1b8cca57cb1d6c726ed9b2590ee5ec6 commit - 39afa5819b25cb2a5a8411de6f16d1e5667bb2c7 commit + b7c8ea3da1b8cca57cb1d6c726ed9b2590ee5ec6 blob - 87aa73baf749f6c131882947826effd5278c6c05 blob + 94e9ec2701b4224f2034e525cf69de75b4a4f65b --- gotwebd/gotweb.c +++ gotwebd/gotweb.c @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -93,9 +94,9 @@ static const struct got_error *gotweb_get_repo_descrip static const struct got_error *gotweb_load_got_path(struct request *c, struct repo_dir *); static const struct got_error *gotweb_get_repo_description(char **, - struct server *, char *); + struct server *, int); static const struct got_error *gotweb_get_clone_url(char **, struct server *, - char *); + int); static const struct got_error *gotweb_render_navs(struct request *); static const struct got_error *gotweb_render_blame(struct request *); static const struct got_error *gotweb_render_briefs(struct request *); @@ -2438,7 +2439,7 @@ done: } t->repo = repo; error = gotweb_get_repo_description(&repo_dir->description, srv, - repo_dir->path); + dirfd(dt)); if (error) goto err; error = got_get_repo_owner(&repo_dir->owner, c); @@ -2447,7 +2448,7 @@ done: error = got_get_repo_age(&repo_dir->age, c, NULL, TM_DIFF); if (error) goto err; - error = gotweb_get_clone_url(&repo_dir->url, srv, repo_dir->path); + error = gotweb_get_clone_url(&repo_dir->url, srv, dirfd(dt)); err: free(dir_test); if (dt != NULL && closedir(dt) == EOF && error == NULL) @@ -2480,119 +2481,94 @@ gotweb_get_repo_description(char **description, struct } static const struct got_error * -gotweb_get_repo_description(char **description, struct server *srv, char *dir) +gotweb_get_repo_description(char **description, struct server *srv, int dir) { const struct got_error *error = NULL; - FILE *f = NULL; - char *d_file = NULL; - unsigned int len; - size_t n; + int fd = -1; + off_t len; *description = NULL; if (srv->show_repo_description == 0) return NULL; - if (asprintf(&d_file, "%s/description", dir) == -1) - return got_error_from_errno("asprintf"); - - f = fopen(d_file, "r"); - if (f == NULL) { + fd = openat(dir, "description", O_RDONLY); + if (fd == -1) { if (errno != ENOENT && errno != EACCES) - error = got_error_from_errno2("fopen", d_file); + error = got_error_from_errno2("openat", "description"); goto done; } - if (fseek(f, 0, SEEK_END) == -1) { - error = got_ferror(f, GOT_ERR_IO); - goto done; - } - len = ftell(f); + len = lseek(fd, 0, SEEK_END); if (len == -1) { - error = got_ferror(f, GOT_ERR_IO); + error = got_error_from_errno("lseek"); goto done; } - if (len == 0) { - *description = strdup(""); - if (*description == NULL) - error = got_error_from_errno("strdup"); + if (lseek(fd, 0, SEEK_SET) == -1) { + error = got_error_from_errno("fseek"); goto done; } - if (fseek(f, 0, SEEK_SET) == -1) { - error = got_ferror(f, GOT_ERR_IO); - goto done; - } + if (len > SIZE_MAX - 1) + len = SIZE_MAX - 1; + *description = calloc(len + 1, sizeof(**description)); if (*description == NULL) { error = got_error_from_errno("calloc"); goto done; } - n = fread(*description, 1, len, f); - if (n == 0 && ferror(f)) - error = got_ferror(f, GOT_ERR_IO); + if (read(fd, *description, len) == -1) + error = got_error_from_errno("read"); done: - if (f != NULL && fclose(f) == EOF && error == NULL) - error = got_error_from_errno("fclose"); - free(d_file); + if (fd != -1 && close(fd) == -1 && error == NULL) + error = got_error_from_errno("close"); return error; } static const struct got_error * -gotweb_get_clone_url(char **url, struct server *srv, char *dir) +gotweb_get_clone_url(char **url, struct server *srv, int dir) { const struct got_error *error = NULL; - FILE *f; - char *d_file = NULL; - unsigned int len; - size_t n; + int fd = -1; + off_t len; *url = NULL; - if (srv->show_repo_cloneurl == 0) return NULL; - if (asprintf(&d_file, "%s/cloneurl", dir) == -1) - return got_error_from_errno("asprintf"); - - f = fopen(d_file, "r"); - if (f == NULL) { + fd = openat(dir, "cloneurl", O_RDONLY); + if (fd == -1) { if (errno != ENOENT && errno != EACCES) - error = got_error_from_errno2("fopen", d_file); + error = got_error_from_errno2("openat", "cloneurl"); goto done; } - if (fseek(f, 0, SEEK_END) == -1) { - error = got_ferror(f, GOT_ERR_IO); - goto done; - } - len = ftell(f); + len = lseek(fd, 0, SEEK_END); if (len == -1) { - error = got_ferror(f, GOT_ERR_IO); + error = got_error_from_errno("lseek"); goto done; } - if (len == 0) - goto done; - if (fseek(f, 0, SEEK_SET) == -1) { - error = got_ferror(f, GOT_ERR_IO); + if (lseek(fd, 0, SEEK_SET) == -1) { + error = got_error_from_errno("lseek"); goto done; } + if (len > SIZE_MAX - 1) + len = SIZE_MAX - 1; + *url = calloc(len + 1, sizeof(**url)); if (*url == NULL) { error = got_error_from_errno("calloc"); goto done; } - n = fread(*url, 1, len, f); - if (n == 0 && ferror(f)) - error = got_ferror(f, GOT_ERR_IO); + if (read(fd, *url, len) == -1) + error = got_error_from_errno("read"); done: - if (f != NULL && fclose(f) == EOF && error == NULL) + if (fd != -1 && close(fd) == -1 && error == NULL) error = got_error_from_errno("fclose"); - free(d_file); return error; }