Download raw body.
llvm scan-build: improve error handling, use-after-free, stack garbage
Hi,
so I ran llvm's scan-build against got and waded through the results.
It produces an imperial buttload of false positives, the report needs to
be carefully analysed.
Let me know if you want this submitted a different way, I could run
it through git send-email to bother you with 11 emails... or maybe group
it by category? There is also more stuff to submit...
commit 016cea2b6e56ea13a98b37edff64213f49162549
Author: Florian Obser <florian@narrans.de>
Date: Wed Jul 20 17:50:13 2022 +0200
Don't chug along if repo format version is unsupported.
Found by llvm's scan-build (dead store).
diff --git lib/repository.c lib/repository.c
index 4c93c601..1df2b67a 100644
--- lib/repository.c
+++ lib/repository.c
@@ -788,8 +788,10 @@ got_repo_open(struct got_repository **repop, const char *path,
err = read_gitconfig(repo, global_gitconfig_path);
if (err)
goto done;
- if (repo->gitconfig_repository_format_version != 0)
+ if (repo->gitconfig_repository_format_version != 0) {
err = got_error_path(path, GOT_ERR_GIT_REPO_FORMAT);
+ goto done;
+ }
for (i = 0; i < repo->nextensions; i++) {
char *ext = repo->extensions[i];
int j, supported = 0;
commit c8ac07903bd06317a3cd10d1c57a6fcddc843e84
Author: Florian Obser <florian@narrans.de>
Date: Wed Jul 20 17:50:13 2022 +0200
Do not ignore I/O errors.
Found by llvm's scan-build (dead store).
diff --git lib/worktree.c lib/worktree.c
index 585486aa..46a800f7 100644
--- lib/worktree.c
+++ lib/worktree.c
@@ -1270,6 +1270,9 @@ install_symlink(int *is_bad_symlink, struct got_worktree *worktree,
*/
do {
err = got_object_blob_read_block(&len, blob);
+ if (err)
+ return err;
+
if (len + target_len >= sizeof(target_path)) {
/* Path too long; install as a regular file. */
*is_bad_symlink = 1;
commit 9259064acd57549e58cbae8b28ed4edf9b395512
Author: Florian Obser <florian@narrans.de>
Date: Wed Jul 20 17:50:14 2022 +0200
path_got is unused and never assigned, no need to free it
diff --git lib/worktree.c lib/worktree.c
index 46a800f7..cc19332e 100644
--- lib/worktree.c
+++ lib/worktree.c
@@ -1256,13 +1256,12 @@ install_symlink(int *is_bad_symlink, struct got_worktree *worktree,
const struct got_error *err = NULL;
char target_path[PATH_MAX];
size_t len, target_len = 0;
- char *path_got = NULL;
const uint8_t *buf = got_object_blob_get_read_buf(blob);
size_t hdrlen = got_object_blob_get_hdrlen(blob);
*is_bad_symlink = 0;
- /*
+ /*
* Blob object content specifies the target path of the link.
* If a symbolic link cannot be installed we instead create
* a regular file which contains the link target path stored
@@ -1305,7 +1304,7 @@ install_symlink(int *is_bad_symlink, struct got_worktree *worktree,
GOT_DEFAULT_FILE_MODE, GOT_DEFAULT_FILE_MODE, blob,
restoring_missing_file, reverting_versioned_file, 1,
path_is_unversioned, repo, progress_cb, progress_arg);
- goto done;
+ return err;
}
if (symlink(target_path, ondisk_path) == -1) {
@@ -1314,12 +1313,12 @@ install_symlink(int *is_bad_symlink, struct got_worktree *worktree,
if (path_is_unversioned) {
err = (*progress_cb)(progress_arg,
GOT_STATUS_UNVERSIONED, path);
- goto done;
+ return err;
}
err = replace_existing_symlink(&symlink_replaced,
ondisk_path, target_path, target_len);
if (err)
- goto done;
+ return err;
if (progress_cb) {
if (symlink_replaced) {
err = (*progress_cb)(progress_arg,
@@ -1331,25 +1330,25 @@ install_symlink(int *is_bad_symlink, struct got_worktree *worktree,
GOT_STATUS_EXISTS, path);
}
}
- goto done; /* Nothing else to do. */
+ return err; /* Nothing else to do. */
}
if (errno == ENOENT) {
char *parent;
err = got_path_dirname(&parent, ondisk_path);
if (err)
- goto done;
+ return err;
err = add_dir_on_disk(worktree, parent);
free(parent);
if (err)
- goto done;
+ return err;
/*
* Retry, and fall through to error handling
* below if this second attempt fails.
*/
if (symlink(target_path, ondisk_path) != -1) {
err = NULL; /* success */
- goto done;
+ return err;
}
}
@@ -1373,8 +1372,6 @@ install_symlink(int *is_bad_symlink, struct got_worktree *worktree,
} else if (progress_cb)
err = (*progress_cb)(progress_arg, reverting_versioned_file ?
GOT_STATUS_REVERT : GOT_STATUS_ADD, path);
-done:
- free(path_got);
return err;
}
commit 19593e5f9ab7ef8780eca7ab3425e9f59f7e495e
Author: Florian Obser <florian@narrans.de>
Date: Wed Jul 20 17:50:14 2022 +0200
Dot not ignore error from got_object_id_str().
Found by llvm's scan-build (dead store).
diff --git got/got.c got/got.c
index 34f6db81..c32dc030 100644
--- got/got.c
+++ got/got.c
@@ -10312,6 +10312,8 @@ cmd_rebase(int argc, char *argv[])
if (error)
goto done;
error = got_object_id_str(&id_str, new_head_commit_id);
+ if (error)
+ goto done;
printf("Forwarding %s to commit %s\n",
got_ref_get_name(branch), id_str);
free(id_str);
commit 51086de45469df22307e8fa82e2fc4b8eec1fba1
Author: Florian Obser <florian@narrans.de>
Date: Wed Jul 20 17:50:14 2022 +0200
Do not ignore error from format_author().
Found by llvm's scan-build (dead store).
diff --git tog/tog.c tog/tog.c
index a76673e4..3418b52d 100644
--- tog/tog.c
+++ tog/tog.c
@@ -2305,6 +2305,8 @@ draw_commits(struct tog_view *view)
author_cols = width;
free(wauthor);
free(author);
+ if (err)
+ goto done;
err = got_object_commit_get_logmsg(&msg0, c);
if (err)
goto done;
commit 7917de614d61d6bfe2914ee85e772f35bcc5c616
Author: Florian Obser <florian@narrans.de>
Date: Wed Jul 20 17:50:14 2022 +0200
Do not ignore error from got_pathlist_append.
Found by llvm's scan-build (dead store).
diff --git libexec/got-send-pack/got-send-pack.c libexec/got-send-pack/got-send-pack.c
index 1f78b772..11e392eb 100644
--- libexec/got-send-pack/got-send-pack.c
+++ libexec/got-send-pack/got-send-pack.c
@@ -389,6 +389,9 @@ send_pack(int fd, struct got_pathlist_head *refs,
goto done;
err = got_pathlist_append(&their_refs, refname, id);
+ if (err)
+ goto done;
+
if (chattygot)
fprintf(stderr, "%s: remote has %s %s\n",
getprogname(), refname, id_str);
commit c9b2304eea293220eba6fe4cbfb99526c1b2616c
Author: Florian Obser <florian@narrans.de>
Date: Wed Jul 20 17:50:15 2022 +0200
We don't care about the length of the read line.
Found by llvm's scan-build (dead store).
diff --git got/got.c got/got.c
index c32dc030..c55c84a5 100644
--- got/got.c
+++ got/got.c
@@ -396,7 +396,6 @@ edit_logmsg(char **logmsg, const char *editor, const char *logmsg_path,
const struct got_error *err = NULL;
char *line = NULL;
size_t linesize = 0;
- ssize_t linelen;
struct stat st, st2;
FILE *fp = NULL;
size_t len, logmsg_len;
@@ -463,7 +462,7 @@ edit_logmsg(char **logmsg, const char *editor, const char *logmsg_path,
}
len = 0;
- while ((linelen = getline(&line, &linesize, fp)) != -1) {
+ while (getline(&line, &linesize, fp) != -1) {
if ((line[0] == '#' || (len == 0 && line[0] == '\n')))
continue; /* remove comments and leading empty lines */
len = strlcat(*logmsg, line, logmsg_len + 1);
@@ -3933,7 +3932,6 @@ match_patch(int *have_match, struct got_commit_object *commit,
const struct got_error *err = NULL;
char *line = NULL;
size_t linesize = 0;
- ssize_t linelen;
regmatch_t regmatch;
*have_match = 0;
@@ -3951,7 +3949,7 @@ match_patch(int *have_match, struct got_commit_object *commit,
goto done;
}
- while ((linelen = getline(&line, &linesize, f)) != -1) {
+ while (getline(&line, &linesize, f) != -1) {
if (regexec(regex, line, 1, ®match, 0) == 0) {
*have_match = 1;
break;
commit 2ad36c4258f03bea5583d5036468534ff40e8a6e
Author: Florian Obser <florian@narrans.de>
Date: Wed Jul 20 17:50:15 2022 +0200
If the first readdir() returns NULL err is uninitialized.
This can't happen in practice, but llvm's scan-build doesn't know this.
diff --git lib/repository_admin.c lib/repository_admin.c
index 173521ee..4eefd4d4 100644
--- lib/repository_admin.c
+++ lib/repository_admin.c
@@ -1239,7 +1239,7 @@ got_repo_remove_lonely_packidx(struct got_repository *repo, int dry_run,
got_lonely_packidx_progress_cb progress_cb, void *progress_arg,
got_cancel_cb cancel_cb, void *cancel_arg)
{
- const struct got_error *err;
+ const struct got_error *err = NULL;
DIR *packdir = NULL;
struct dirent *dent;
char *pack_relpath = NULL;
commit f2f31d69eadae22cce81602745b85ddc56c5789b
Author: Florian Obser <florian@narrans.de>
Date: Wed Jul 20 17:50:15 2022 +0200
Make sure got_repo_pack_fds_close() frees a malloc'ed pointer.
Found by llvm's scan-build (bad free).
diff --git lib/repository.c lib/repository.c
index 1df2b67a..1afcaaff 100644
--- lib/repository.c
+++ lib/repository.c
@@ -252,11 +252,16 @@ const struct got_error *
got_repo_pack_fds_open(int **pack_fds)
{
const struct got_error *err = NULL;
- int i, pack_fds_tmp[GOT_PACK_NUM_TEMPFILES];
+ int i, *pack_fds_tmp;
+ pack_fds_tmp = calloc(GOT_PACK_NUM_TEMPFILES, sizeof(int));
+ if (pack_fds_tmp == NULL)
+ return got_error_from_errno("calloc");
*pack_fds = calloc(GOT_PACK_NUM_TEMPFILES, sizeof(**pack_fds));
- if (*pack_fds == NULL)
+ if (*pack_fds == NULL) {
+ free(pack_fds_tmp);
return got_error_from_errno("calloc");
+ }
for (i = 0; i < GOT_PACK_NUM_TEMPFILES; i++) {
pack_fds_tmp[i] = got_opentempfd();
@@ -266,7 +271,7 @@ got_repo_pack_fds_open(int **pack_fds)
return err;
}
}
- memcpy(*pack_fds, pack_fds_tmp, sizeof(pack_fds_tmp));
+ memcpy(*pack_fds, pack_fds_tmp, GOT_PACK_NUM_TEMPFILES * sizeof(int));
return err;
}
commit cecbaa82760f08c70563aa7ae6bd3cdc1b273be8
Author: Florian Obser <florian@narrans.de>
Date: Wed Jul 20 17:50:15 2022 +0200
Prevent memory leak when asprintf fails.
Found by llvm's scan-build.
diff --git lib/object_create.c lib/object_create.c
index e7a5a231..05652e30 100644
--- lib/object_create.c
+++ lib/object_create.c
@@ -454,8 +454,10 @@ got_object_commit_create(struct got_object_id **id,
}
if (asprintf(&author_str, "%s%s %lld +0000\n",
- GOT_COMMIT_LABEL_AUTHOR, author, (long long)author_time) == -1)
- return got_error_from_errno("asprintf");
+ GOT_COMMIT_LABEL_AUTHOR, author, (long long)author_time) == -1) {
+ err = got_error_from_errno("asprintf");
+ goto done;
+ }
if (asprintf(&committer_str, "%s%s %lld +0000\n",
GOT_COMMIT_LABEL_COMMITTER, committer ? committer : author,
commit 46b3c37e25b39ee15fcebad30efedf66d56d1232
Author: Florian Obser <florian@narrans.de>
Date: Wed Jul 20 17:50:16 2022 +0200
Prevent use-after-free of packed_refs_path in error path.
Found by llvm's scan-build.
diff --git lib/reference.c lib/reference.c
index de71b584..78eb9eba 100644
--- lib/reference.c
+++ lib/reference.c
@@ -453,7 +453,7 @@ got_ref_open(struct got_reference **ref, struct got_repository *repo,
const char *refname, int lock)
{
const struct got_error *err = NULL;
- char *path_refs = NULL;
+ char *packed_refs_path = NULL, *path_refs = NULL;
const char *subdirs[] = {
GOT_REF_HEADS, GOT_REF_TAGS, GOT_REF_REMOTES
};
@@ -472,7 +472,6 @@ got_ref_open(struct got_reference **ref, struct got_repository *repo,
if (well_known) {
err = open_ref(ref, path_refs, "", refname, lock);
} else {
- char *packed_refs_path;
FILE *f;
/* Search on-disk refs before packed refs! */
@@ -496,7 +495,6 @@ got_ref_open(struct got_reference **ref, struct got_repository *repo,
goto done;
}
f = fopen(packed_refs_path, "rbe");
- free(packed_refs_path);
if (f != NULL) {
struct stat sb;
if (fstat(fileno(f), &sb) == -1) {
@@ -521,6 +519,7 @@ done:
err = got_error_not_ref(refname);
if (err && lf)
got_lockfile_unlock(lf, -1);
+ free(packed_refs_path);
free(path_refs);
return err;
}
@@ -997,7 +996,7 @@ got_ref_list(struct got_reflist_head *refs, struct got_repository *repo,
const char *ref_namespace, got_ref_cmp_cb cmp_cb, void *cmp_arg)
{
const struct got_error *err;
- char *packed_refs_path, *path_refs = NULL;
+ char *packed_refs_path = NULL, *path_refs = NULL;
char *abs_namespace = NULL, *buf = NULL;
const char *ondisk_ref_namespace = NULL;
char *line = NULL;
@@ -1090,7 +1089,6 @@ got_ref_list(struct got_reflist_head *refs, struct got_repository *repo,
}
f = fopen(packed_refs_path, "re");
- free(packed_refs_path);
if (f) {
size_t linesize = 0;
ssize_t linelen;
@@ -1135,6 +1133,7 @@ got_ref_list(struct got_reflist_head *refs, struct got_repository *repo,
}
}
done:
+ free(packed_refs_path);
free(abs_namespace);
free(buf);
free(line);
--
I'm not entirely sure you are real.
llvm scan-build: improve error handling, use-after-free, stack garbage