subject:"Re\: \[PATCH v5 18\/21\] commit\-graph\: use string\-list API for input"

Re: [PATCH v5 18/21] commit-graph: use string-list API for input

2018-06-08 Thread Derrick Stolee


On 6/6/2018 8:45 AM, Derrick Stolee wrote:

On 6/6/2018 8:26 AM, Ævar Arnfjörð Bjarmason wrote:

On Wed, Jun 06 2018, Derrick Stolee wrote:


On 6/6/2018 8:11 AM, Ævar Arnfjörð Bjarmason wrote:

On Wed, Jun 06 2018, Derrick Stolee wrote:


Signed-off-by: Derrick Stolee 

+    string_list_clear(, 0);
   return 0;
   }

This results in an invalid free() & segfault because you're freeing
 which may not have been allocated by string_list_init().

Good point. Did my tests not catch this? (seems it requires calling
`git commit-graph write` with no `--stdin-packs` or
`--stdin-commits`).

Most of your tests (t5318-commit-graph.sh) segfaulted, but presumably
you're on a more forgiving compiler/platform/options. I compiled with
-O0 -g on clang 4.0.1-8 + Debian testing.


I appreciate the extra platform testing. I'm using GCC on Ubuntu (gcc 
(Ubuntu 7.3.0-16ubuntu3) 7.3.0).


While the errors didn't repro on my box, they did on the VSTS Linux 
build machines [1]. I created an internal PR just so I could run those 
tests (and on our OSX agents which use clang) and so I can verify I 
fixed the situation. I'll send a v6 soon so Junio only picks up a 
version that succeeds in tests.


Thanks,
-Stolee

[1] 
https://github.com/Microsoft/vsts-agent-docker/blob/master/ubuntu/16.04/standard/Dockerfile

    What's installed on a VSTS Linux build agent

Re: [PATCH v5 18/21] commit-graph: use string-list API for input

2018-06-06 Thread Derrick Stolee


On 6/6/2018 8:26 AM, Ævar Arnfjörð Bjarmason wrote:

On Wed, Jun 06 2018, Derrick Stolee wrote:


On 6/6/2018 8:11 AM, Ævar Arnfjörð Bjarmason wrote:

On Wed, Jun 06 2018, Derrick Stolee wrote:


Signed-off-by: Derrick Stolee 
---
   builtin/commit-graph.c | 39 +--
   commit-graph.c | 15 +++
   commit-graph.h |  7 +++
   3 files changed, 23 insertions(+), 38 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 3079cde6f9..d8eb8278b3 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -118,13 +118,9 @@ static int graph_read(int argc, const char **argv)

   static int graph_write(int argc, const char **argv)
   {
-   const char **pack_indexes = NULL;
-   int packs_nr = 0;
-   const char **commit_hex = NULL;
-   int commits_nr = 0;
-   const char **lines = NULL;
-   int lines_nr = 0;
-   int lines_alloc = 0;
+   struct string_list *pack_indexes = NULL;
+   struct string_list *commit_hex = NULL;
+   struct string_list lines;

static struct option builtin_commit_graph_write_options[] = {
OPT_STRING(0, "object-dir", _dir,
@@ -150,32 +146,23 @@ static int graph_write(int argc, const char **argv)

if (opts.stdin_packs || opts.stdin_commits) {
struct strbuf buf = STRBUF_INIT;
-   lines_nr = 0;
-   lines_alloc = 128;
-   ALLOC_ARRAY(lines, lines_alloc);
-
-   while (strbuf_getline(, stdin) != EOF) {
-   ALLOC_GROW(lines, lines_nr + 1, lines_alloc);
-   lines[lines_nr++] = strbuf_detach(, NULL);
-   }
-
-   if (opts.stdin_packs) {
-   pack_indexes = lines;
-   packs_nr = lines_nr;
-   }
-   if (opts.stdin_commits) {
-   commit_hex = lines;
-   commits_nr = lines_nr;
-   }
+   string_list_init(, 0);
+
+   while (strbuf_getline(, stdin) != EOF)
+   string_list_append(, strbuf_detach(, NULL));
+
+   if (opts.stdin_packs)
+   pack_indexes = 
+   if (opts.stdin_commits)
+   commit_hex = 
}

write_commit_graph(opts.obj_dir,
   pack_indexes,
-  packs_nr,
   commit_hex,
-  commits_nr,
   opts.append);

+   string_list_clear(, 0);
return 0;
   }

This results in an invalid free() & segfault because you're freeing
 which may not have been allocated by string_list_init().

Good point. Did my tests not catch this? (seems it requires calling
`git commit-graph write` with no `--stdin-packs` or
`--stdin-commits`).

Most of your tests (t5318-commit-graph.sh) segfaulted, but presumably
you're on a more forgiving compiler/platform/options. I compiled with
-O0 -g on clang 4.0.1-8 + Debian testing.


I appreciate the extra platform testing. I'm using GCC on Ubuntu (gcc 
(Ubuntu 7.3.0-16ubuntu3) 7.3.0).





Monkeypatch on top which I used to fix it:

  diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
  index 76423b3fa5..c7eb68aa3a 100644
  --- a/builtin/commit-graph.c
  +++ b/builtin/commit-graph.c
  @@ -122,6 +122,7 @@ static int graph_write(int argc, const char **argv)
  struct string_list *pack_indexes = NULL;
  struct string_list *commit_hex = NULL;
  struct string_list lines;
  +   int free_lines = 0;

  static struct option builtin_commit_graph_write_options[] = {
  OPT_STRING(0, "object-dir", _dir,
  @@ -155,6 +156,7 @@ static int graph_write(int argc, const char **argv)
  if (opts.stdin_packs || opts.stdin_commits) {
  struct strbuf buf = STRBUF_INIT;
  string_list_init(, 0);
  +   free_lines = 1;

  while (strbuf_getline(, stdin) != EOF)
  string_list_append(, strbuf_detach(, 
NULL));
  @@ -170,7 +172,8 @@ static int graph_write(int argc, const char **argv)
 commit_hex,
 opts.append);

  -   string_list_clear(, 0);
  +   if (free_lines)
  +   string_list_clear(, 0);
  return 0;
   }

But probably having a pointer to the struct which is NULL etc. is
better.

Wouldn't the easiest fix be to call `string_list_init(, 0)`
outside of any conditional?

Sure that works too. We'd be doing the init when we don't need it, but
it's not like this part is performance critical or anything...

Re: [PATCH v5 18/21] commit-graph: use string-list API for input

2018-06-06 Thread Ævar Arnfjörð Bjarmason



On Wed, Jun 06 2018, Derrick Stolee wrote:

> On 6/6/2018 8:11 AM, Ævar Arnfjörð Bjarmason wrote:
>> On Wed, Jun 06 2018, Derrick Stolee wrote:
>>
>>> Signed-off-by: Derrick Stolee 
>>> ---
>>>   builtin/commit-graph.c | 39 +--
>>>   commit-graph.c | 15 +++
>>>   commit-graph.h |  7 +++
>>>   3 files changed, 23 insertions(+), 38 deletions(-)
>>> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
>>> index 3079cde6f9..d8eb8278b3 100644
>>> --- a/builtin/commit-graph.c
>>> +++ b/builtin/commit-graph.c
>>> @@ -118,13 +118,9 @@ static int graph_read(int argc, const char **argv)
>>>
>>>   static int graph_write(int argc, const char **argv)
>>>   {
>>> -   const char **pack_indexes = NULL;
>>> -   int packs_nr = 0;
>>> -   const char **commit_hex = NULL;
>>> -   int commits_nr = 0;
>>> -   const char **lines = NULL;
>>> -   int lines_nr = 0;
>>> -   int lines_alloc = 0;
>>> +   struct string_list *pack_indexes = NULL;
>>> +   struct string_list *commit_hex = NULL;
>>> +   struct string_list lines;
>>>
>>> static struct option builtin_commit_graph_write_options[] = {
>>> OPT_STRING(0, "object-dir", _dir,
>>> @@ -150,32 +146,23 @@ static int graph_write(int argc, const char **argv)
>>>
>>> if (opts.stdin_packs || opts.stdin_commits) {
>>> struct strbuf buf = STRBUF_INIT;
>>> -   lines_nr = 0;
>>> -   lines_alloc = 128;
>>> -   ALLOC_ARRAY(lines, lines_alloc);
>>> -
>>> -   while (strbuf_getline(, stdin) != EOF) {
>>> -   ALLOC_GROW(lines, lines_nr + 1, lines_alloc);
>>> -   lines[lines_nr++] = strbuf_detach(, NULL);
>>> -   }
>>> -
>>> -   if (opts.stdin_packs) {
>>> -   pack_indexes = lines;
>>> -   packs_nr = lines_nr;
>>> -   }
>>> -   if (opts.stdin_commits) {
>>> -   commit_hex = lines;
>>> -   commits_nr = lines_nr;
>>> -   }
>>> +   string_list_init(, 0);
>>> +
>>> +   while (strbuf_getline(, stdin) != EOF)
>>> +   string_list_append(, strbuf_detach(, NULL));
>>> +
>>> +   if (opts.stdin_packs)
>>> +   pack_indexes = 
>>> +   if (opts.stdin_commits)
>>> +   commit_hex = 
>>> }
>>>
>>> write_commit_graph(opts.obj_dir,
>>>pack_indexes,
>>> -  packs_nr,
>>>commit_hex,
>>> -  commits_nr,
>>>opts.append);
>>>
>>> +   string_list_clear(, 0);
>>> return 0;
>>>   }
>> This results in an invalid free() & segfault because you're freeing
>>  which may not have been allocated by string_list_init().
>
> Good point. Did my tests not catch this? (seems it requires calling
> `git commit-graph write` with no `--stdin-packs` or
> `--stdin-commits`).

Most of your tests (t5318-commit-graph.sh) segfaulted, but presumably
you're on a more forgiving compiler/platform/options. I compiled with
-O0 -g on clang 4.0.1-8 + Debian testing.

>>
>> Monkeypatch on top which I used to fix it:
>>
>>  diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
>>  index 76423b3fa5..c7eb68aa3a 100644
>>  --- a/builtin/commit-graph.c
>>  +++ b/builtin/commit-graph.c
>>  @@ -122,6 +122,7 @@ static int graph_write(int argc, const char **argv)
>>  struct string_list *pack_indexes = NULL;
>>  struct string_list *commit_hex = NULL;
>>  struct string_list lines;
>>  +   int free_lines = 0;
>>
>>  static struct option builtin_commit_graph_write_options[] = {
>>  OPT_STRING(0, "object-dir", _dir,
>>  @@ -155,6 +156,7 @@ static int graph_write(int argc, const char **argv)
>>  if (opts.stdin_packs || opts.stdin_commits) {
>>  struct strbuf buf = STRBUF_INIT;
>>  string_list_init(, 0);
>>  +   free_lines = 1;
>>
>>  while (strbuf_getline(, stdin) != EOF)
>>  string_list_append(, strbuf_detach(, 
>> NULL));
>>  @@ -170,7 +172,8 @@ static int graph_write(int argc, const char **argv)
>> commit_hex,
>> opts.append);
>>
>>  -   string_list_clear(, 0);
>>  +   if (free_lines)
>>  +   string_list_clear(, 0);
>>  return 0;
>>   }
>>
>> But probably having a pointer to the struct which is NULL etc. is
>> better.
>
> Wouldn't the easiest fix be to call `string_list_init(, 0)`
> outside of any conditional?

Sure that works too. We'd be doing the init when we don't need it, but
it's not like this part is performance critical or anything...

Re: [PATCH v5 18/21] commit-graph: use string-list API for input

2018-06-06 Thread Derrick Stolee


On 6/6/2018 8:11 AM, Ævar Arnfjörð Bjarmason wrote:

On Wed, Jun 06 2018, Derrick Stolee wrote:


Signed-off-by: Derrick Stolee 
---
  builtin/commit-graph.c | 39 +--
  commit-graph.c | 15 +++
  commit-graph.h |  7 +++
  3 files changed, 23 insertions(+), 38 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 3079cde6f9..d8eb8278b3 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -118,13 +118,9 @@ static int graph_read(int argc, const char **argv)

  static int graph_write(int argc, const char **argv)
  {
-   const char **pack_indexes = NULL;
-   int packs_nr = 0;
-   const char **commit_hex = NULL;
-   int commits_nr = 0;
-   const char **lines = NULL;
-   int lines_nr = 0;
-   int lines_alloc = 0;
+   struct string_list *pack_indexes = NULL;
+   struct string_list *commit_hex = NULL;
+   struct string_list lines;

static struct option builtin_commit_graph_write_options[] = {
OPT_STRING(0, "object-dir", _dir,
@@ -150,32 +146,23 @@ static int graph_write(int argc, const char **argv)

if (opts.stdin_packs || opts.stdin_commits) {
struct strbuf buf = STRBUF_INIT;
-   lines_nr = 0;
-   lines_alloc = 128;
-   ALLOC_ARRAY(lines, lines_alloc);
-
-   while (strbuf_getline(, stdin) != EOF) {
-   ALLOC_GROW(lines, lines_nr + 1, lines_alloc);
-   lines[lines_nr++] = strbuf_detach(, NULL);
-   }
-
-   if (opts.stdin_packs) {
-   pack_indexes = lines;
-   packs_nr = lines_nr;
-   }
-   if (opts.stdin_commits) {
-   commit_hex = lines;
-   commits_nr = lines_nr;
-   }
+   string_list_init(, 0);
+
+   while (strbuf_getline(, stdin) != EOF)
+   string_list_append(, strbuf_detach(, NULL));
+
+   if (opts.stdin_packs)
+   pack_indexes = 
+   if (opts.stdin_commits)
+   commit_hex = 
}

write_commit_graph(opts.obj_dir,
   pack_indexes,
-  packs_nr,
   commit_hex,
-  commits_nr,
   opts.append);

+   string_list_clear(, 0);
return 0;
  }

This results in an invalid free() & segfault because you're freeing
 which may not have been allocated by string_list_init().


Good point. Did my tests not catch this? (seems it requires calling `git 
commit-graph write` with no `--stdin-packs` or `--stdin-commits`).




Monkeypatch on top which I used to fix it:

 diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
 index 76423b3fa5..c7eb68aa3a 100644
 --- a/builtin/commit-graph.c
 +++ b/builtin/commit-graph.c
 @@ -122,6 +122,7 @@ static int graph_write(int argc, const char **argv)
 struct string_list *pack_indexes = NULL;
 struct string_list *commit_hex = NULL;
 struct string_list lines;
 +   int free_lines = 0;

 static struct option builtin_commit_graph_write_options[] = {
 OPT_STRING(0, "object-dir", _dir,
 @@ -155,6 +156,7 @@ static int graph_write(int argc, const char **argv)
 if (opts.stdin_packs || opts.stdin_commits) {
 struct strbuf buf = STRBUF_INIT;
 string_list_init(, 0);
 +   free_lines = 1;

 while (strbuf_getline(, stdin) != EOF)
 string_list_append(, strbuf_detach(, 
NULL));
 @@ -170,7 +172,8 @@ static int graph_write(int argc, const char **argv)
commit_hex,
opts.append);

 -   string_list_clear(, 0);
 +   if (free_lines)
 +   string_list_clear(, 0);
 return 0;
  }

But probably having a pointer to the struct which is NULL etc. is
better.


Wouldn't the easiest fix be to call `string_list_init(, 0)` 
outside of any conditional?


Thanks,
-Stolee

Re: [PATCH v5 18/21] commit-graph: use string-list API for input

2018-06-06 Thread Ævar Arnfjörð Bjarmason



On Wed, Jun 06 2018, Derrick Stolee wrote:

> Signed-off-by: Derrick Stolee 
> ---
>  builtin/commit-graph.c | 39 +--
>  commit-graph.c | 15 +++
>  commit-graph.h |  7 +++
>  3 files changed, 23 insertions(+), 38 deletions(-)

> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 3079cde6f9..d8eb8278b3 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -118,13 +118,9 @@ static int graph_read(int argc, const char **argv)
>
>  static int graph_write(int argc, const char **argv)
>  {
> - const char **pack_indexes = NULL;
> - int packs_nr = 0;
> - const char **commit_hex = NULL;
> - int commits_nr = 0;
> - const char **lines = NULL;
> - int lines_nr = 0;
> - int lines_alloc = 0;
> + struct string_list *pack_indexes = NULL;
> + struct string_list *commit_hex = NULL;
> + struct string_list lines;
>
>   static struct option builtin_commit_graph_write_options[] = {
>   OPT_STRING(0, "object-dir", _dir,
> @@ -150,32 +146,23 @@ static int graph_write(int argc, const char **argv)
>
>   if (opts.stdin_packs || opts.stdin_commits) {
>   struct strbuf buf = STRBUF_INIT;
> - lines_nr = 0;
> - lines_alloc = 128;
> - ALLOC_ARRAY(lines, lines_alloc);
> -
> - while (strbuf_getline(, stdin) != EOF) {
> - ALLOC_GROW(lines, lines_nr + 1, lines_alloc);
> - lines[lines_nr++] = strbuf_detach(, NULL);
> - }
> -
> - if (opts.stdin_packs) {
> - pack_indexes = lines;
> - packs_nr = lines_nr;
> - }
> - if (opts.stdin_commits) {
> - commit_hex = lines;
> - commits_nr = lines_nr;
> - }
> + string_list_init(, 0);
> +
> + while (strbuf_getline(, stdin) != EOF)
> + string_list_append(, strbuf_detach(, NULL));
> +
> + if (opts.stdin_packs)
> + pack_indexes = 
> + if (opts.stdin_commits)
> + commit_hex = 
>   }
>
>   write_commit_graph(opts.obj_dir,
>  pack_indexes,
> -packs_nr,
>  commit_hex,
> -commits_nr,
>  opts.append);
>
> + string_list_clear(, 0);
>   return 0;
>  }

This results in an invalid free() & segfault because you're freeing
 which may not have been allocated by string_list_init().

Monkeypatch on top which I used to fix it:

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 76423b3fa5..c7eb68aa3a 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -122,6 +122,7 @@ static int graph_write(int argc, const char **argv)
struct string_list *pack_indexes = NULL;
struct string_list *commit_hex = NULL;
struct string_list lines;
+   int free_lines = 0;

static struct option builtin_commit_graph_write_options[] = {
OPT_STRING(0, "object-dir", _dir,
@@ -155,6 +156,7 @@ static int graph_write(int argc, const char **argv)
if (opts.stdin_packs || opts.stdin_commits) {
struct strbuf buf = STRBUF_INIT;
string_list_init(, 0);
+   free_lines = 1;

while (strbuf_getline(, stdin) != EOF)
string_list_append(, strbuf_detach(, 
NULL));
@@ -170,7 +172,8 @@ static int graph_write(int argc, const char **argv)
   commit_hex,
   opts.append);

-   string_list_clear(, 0);
+   if (free_lines)
+   string_list_clear(, 0);
return 0;
 }

But probably having a pointer to the struct which is NULL etc. is
better.

Re: [PATCH v5 18/21] commit-graph: use string-list API for input

Re: [PATCH v5 18/21] commit-graph: use string-list API for input

Re: [PATCH v5 18/21] commit-graph: use string-list API for input

Re: [PATCH v5 18/21] commit-graph: use string-list API for input

Re: [PATCH v5 18/21] commit-graph: use string-list API for input

5 matches

Site Navigation

Mail list logo

Footer information