On 24/09/16 01:45PM, Patrick Steinhardt wrote:
> In `get_html_page_path()` we may end up assigning the return value of
> `system_path()` to the global `html_path` variable. But as we also
> assign the returned value to `to_free`, we will deallocate its memory
> upon returning from the function. Consequently, `html_path` will now
> point to deallocated memory.
>
> Fix this issue by instead assigning the value to a separate local
> variable.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> builtin/help.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/help.c b/builtin/help.c
> index dc1fbe2b986..282ea5721fa 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -513,23 +513,24 @@ static void show_info_page(const char *page)
> static void get_html_page_path(struct strbuf *page_path, const char *page)
> {
> struct stat st;
> + const char *path = html_path;
> char *to_free = NULL;
>
> - if (!html_path)
> - html_path = to_free = system_path(GIT_HTML_PATH);
> + if (!path)
> + path = to_free = system_path(GIT_HTML_PATH);
Previously, `html_path` was being assigned the return value of
`system_path()` and consequently escaping this scope even though the
value was being freed. There is no reason to modify the global value to
begin with so we instead assign the value to a local variable. Makes
sense.
>
> /*
> * Check that the page we're looking for exists.
> */
> - if (!strstr(html_path, "://")) {
> - if (stat(mkpath("%s/%s.html", html_path, page), &st)
> + if (!strstr(path, "://")) {
> + if (stat(mkpath("%s/%s.html", path, page), &st)
> || !S_ISREG(st.st_mode))
> die("'%s/%s.html': documentation file not found.",
> - html_path, page);
> + path, page);
> }
>
> strbuf_init(page_path, 0);
> - strbuf_addf(page_path, "%s/%s.html", html_path, page);
> + strbuf_addf(page_path, "%s/%s.html", path, page);
> free(to_free);
> }
>
> --
> 2.46.0.551.gc5ee8f2d1c.dirty
>
>
