| Message ID | RFC-patch-02.15-4a055969ea5-20220603T183608Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix GCC -fanalyzer warnings & add -fanalyzer DEVOPTS mode | expand |
Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason: > Fix an issue with feeding NULL to strcmp() noted by GCC's -fanalyzer, > this fixes a bug in 1678b81ecce (pull: teach git pull about --rebase, > 2015-06-18). > > In cmd_pull() we could go through the function without initializing > the "repo" argument (the -fanalyzer output shows how exactly), we'd > then call get_rebase_fork_point (), which would in turn call > get_tracking_branch() with that "NULL" repo argument. > > Let's avoid this potential issue by returning NULL in this case, which > will have get_rebase_fork_point() return -1 in turn. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/pull.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/builtin/pull.c b/builtin/pull.c > index 01155ba67b2..ed8df004028 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -756,14 +756,16 @@ static const char *get_tracking_branch(const char *remote, const char *refspec) > starts_with(spec_src, "remotes/")) > spec_src = ""; > > - if (*spec_src) { > - if (!strcmp(remote, ".")) > - merge_branch = mkpath("refs/heads/%s", spec_src); > - else > - merge_branch = mkpath("refs/remotes/%s/%s", remote, spec_src); > - } else > + if ((*spec_src && !remote) || !*spec_src) { This is equivalent to: + if (!*spec_src || !remote) { It's easier to read without the redundant term. The patch would be even easier to understand as a one-liner that just adds the missing check, i.e.: - if (*spec_src) { + if (*spec_src && remote) { > merge_branch = NULL; > + goto cleanup; > + } > > + if (!strcmp(remote, ".")) > + merge_branch = mkpath("refs/heads/%s", spec_src); > + else > + merge_branch = mkpath("refs/remotes/%s/%s", remote, spec_src); > +cleanup: > refspec_item_clear(&spec); > return merge_branch; > }
diff --git a/builtin/pull.c b/builtin/pull.c index 01155ba67b2..ed8df004028 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -756,14 +756,16 @@ static const char *get_tracking_branch(const char *remote, const char *refspec) starts_with(spec_src, "remotes/")) spec_src = ""; - if (*spec_src) { - if (!strcmp(remote, ".")) - merge_branch = mkpath("refs/heads/%s", spec_src); - else - merge_branch = mkpath("refs/remotes/%s/%s", remote, spec_src); - } else + if ((*spec_src && !remote) || !*spec_src) { merge_branch = NULL; + goto cleanup; + } + if (!strcmp(remote, ".")) + merge_branch = mkpath("refs/heads/%s", spec_src); + else + merge_branch = mkpath("refs/remotes/%s/%s", remote, spec_src); +cleanup: refspec_item_clear(&spec); return merge_branch; }
Fix an issue with feeding NULL to strcmp() noted by GCC's -fanalyzer, this fixes a bug in 1678b81ecce (pull: teach git pull about --rebase, 2015-06-18). In cmd_pull() we could go through the function without initializing the "repo" argument (the -fanalyzer output shows how exactly), we'd then call get_rebase_fork_point (), which would in turn call get_tracking_branch() with that "NULL" repo argument. Let's avoid this potential issue by returning NULL in this case, which will have get_rebase_fork_point() return -1 in turn. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/pull.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)