git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: SANITIZE=address failure on master (was: [PATCH v8 3/5] config: learn `git_protected_config()`)

From
Glen Choo <chooglen@google.com>
Date
Jul 25, 2022, 20:56 UTC
Message-ID
<kl6lzggwsyh1.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To
<220725.86o7xcords.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 123 quoted lines
> On Mon, Jul 25 2022, Glen Choo wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> On Thu, Jul 14 2022, Glen Choo via GitGitGadget wrote:
>>>
>>>> +/* Read values into protected_config. */
>>>> +static void read_protected_config(void)
>>>> +{
>>>> +	char *xdg_config = NULL, *user_config = NULL, *system_config = NULL;
>>>> +
>>>> +	git_configset_init(&protected_config);
>>>> +
>>>> +	system_config = git_system_config();
>>>> +	git_global_config(&user_config, &xdg_config);
>>>> +
>>>> +	git_configset_add_file(&protected_config, system_config);
>>>> +	git_configset_add_file(&protected_config, xdg_config);
>>>> +	git_configset_add_file(&protected_config, user_config);
>>>> +	git_configset_add_parameters(&protected_config);
>>>> +
>>>> +	free(system_config);
>>>> +	free(xdg_config);
>>>> +	free(user_config);
>>>> +}
>>>
>>> Noticed after it landed on master: This change fails with:
>>>
>>> 	make SANITIZE=address test T=t0410*.sh
>>>
>>> Running that manually shows that we fail like this:
>>> 	
>>> 	$ cat trash\ directory.t0410-partial-clone/httpd/error.log | grep -o AH0.*
>>> 	AH00163: Apache/2.4.54 (Debian) configured -- resuming normal operations
>>> 	AH00094: Command line: '/usr/sbin/apache2 -d /home/avar/g/git/t/trash directory.t0410-partial-clone/httpd -f /home/avar/g/git/t/lib-httpd/apache.conf -c Listen 127.0.0.1:10410'
>>> 	AH01215: AddressSanitizer:DEADLYSIGNAL: /home/avar/g/git/git-http-backend
>>> 	AH01215: =================================================================: /home/avar/g/git/git-http-backend
>>> 	AH01215: ==27820==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f7af5dc0d66 bp 0x7fff11964450 sp 0x7fff11963be8 T0): /home/avar/g/git/git-http-backend
>>> 	AH01215: ==27820==The signal is caused by a READ memory access.: /home/avar/g/git/git-http-backend
>>> 	AH01215: ==27820==Hint: address points to the zero page.: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #0 0x7f7af5dc0d66 in __sanitizer::internal_strlen(char const*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_libc.cpp:167: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #1 0x7f7af5d512f2 in __interceptor_fopen64 ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:6220: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #2 0x562a65e37cc8 in git_fopen compat/fopen.c:22: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #3 0x562a65df3879 in fopen_or_warn wrapper.c:431: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #4 0x562a65a12476 in git_config_from_file_with_options config.c:1982: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #5 0x562a65a124f4 in git_config_from_file config.c:1993: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #6 0x562a65a15288 in git_configset_add_file config.c:2389: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #7 0x562a65a16a37 in read_protected_config config.c:2649: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #8 0x562a65a16b5c in git_protected_config config.c:2661: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #9 0x562a65dd9f9a in get_upload_pack_config upload-pack.c:1342: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #10 0x562a65ddc1cb in upload_pack_v2 upload-pack.c:1706: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #11 0x562a65d2eb8a in process_request serve.c:308: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #12 0x562a65d2ec18 in protocol_v2_serve_loop serve.c:323: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #13 0x562a6593c5ae in cmd_upload_pack builtin/upload-pack.c:55: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #14 0x562a656cf8ff in run_builtin git.c:466: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #15 0x562a656d02ab in handle_builtin git.c:720: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #16 0x562a656d09d5 in run_argv git.c:787: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #17 0x562a656d174f in cmd_main git.c:920: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #18 0x562a6594b0b9 in main common-main.c:56: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #19 0x7f7af5a5681c in __libc_start_main ../csu/libc-start.c:332: /home/avar/g/git/git-http-backend
>>> 	AH01215:     #20 0x562a656cb209 in _start (git+0x1d1209): /home/avar/g/git/git-http-backend
>>> 	AH01215: : /home/avar/g/git/git-http-backend
>>> 	AH01215: AddressSanitizer can not provide additional info.: /home/avar/g/git/git-http-backend
>>> 	AH01215: SUMMARY: AddressSanitizer: SEGV
>>> ../../../../src/libsanitizer/sanitizer_common/sanitizer_libc.cpp:167
>>> in __sanitizer::internal_strlen(char const*):
>>> /home/avar/g/git/git-http-backend
>>> 	AH01215: ==27820==ABORTING: /home/avar/g/git/git-http-backend
>>> 	AH01215: error: upload-pack died of signal 6: /home/avar/g/git/git-http-backend
>>>
>>> (We really should have a SANITIZE=address in CI, but it takes a while...)
>>
>> Thanks. I narrowed the failure down to the hunk above, specifically this
>> line:
>>
>>   git_configset_add_file(&protected_config, xdg_config);
>>
>> Since xdg_config can be NULL, this results in the failing call
>> fopen_or_warn(NULL, "r").
>>
>> This logic was lifted  from do_git_config_sequence(), which checks that
>> each of the paths are not NULL. So a fix might be something like:
>>
>> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
>>
>>   diff --git a/config.c b/config.c
>>   index 015bec360f..208a3dd7a7 100644
>>   --- a/config.c
>>   +++ b/config.c
>>   @@ -2645,9 +2645,13 @@ static void read_protected_config(void)
>>     system_config = git_system_config();
>>     git_global_config(&user_config, &xdg_config);
>>
>>   -	git_configset_add_file(&protected_config, system_config);
>>   -	git_configset_add_file(&protected_config, xdg_config);
>>   -	git_configset_add_file(&protected_config, user_config);
>>   +
>>   +	if (system_config)
>>   +		git_configset_add_file(&protected_config, system_config);
>>   +	if (xdg_config)
>>   +		git_configset_add_file(&protected_config, xdg_config);
>>   +	if (user_config)
>>   +		git_configset_add_file(&protected_config, user_config);
>>     git_configset_add_parameters(&protected_config);
>>
>>     free(system_config);
>>
>> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
>>
>> I'm not sure if system_config can ever be NULL, but (xdg|user)_config is
>> NULL when $HOME is unset, and xdg_config is also unset if
>> $GIT_CONFIG_GLOBAL is set.
>
> Not having looked into it much at all: Doesn't this then introduce
> another logic error where git_protected_config() is now buggy, i.e. it's
> a "lazy load" method where we'll expect to read_protected_config()
> first.
>
> The assumption with that seems to have been that it's invariant within a
> single process, is that still the case, or can e.g. HOME be set during
> our runtime when we rely on these functions?
>
> (I don't know)

I don't think this introduces an error, or at least, not one that we don't already have. This mimics do_git_config_sequence() (which also assumes this invariant), which is used under the hood by (git|repo)_read_config(),

In retrospect, it might have been a good idea to implement read_protected_config() using do_git_config_sequence() / config_with_options(); those functions are a bit bloated, but at least we'd only have one implementation.

Previous: Ævar Arnfjörð BjarmasonNext: Glen Choo via GitGitGadget
Message 111 of 113 in “[RFC] setup.c: make bare repo discovery optional”
  1. [RFC] setup.c: make bare repo discovery optionalGlen Choo via GitGitGadget, May 6, 2022
  2. Junio C HamanoMay 6, 2022
  3. Taylor BlauMay 9, 2022
  4. Junio C HamanoMay 9, 2022
  5. Taylor BlauMay 9, 2022
  6. Junio C HamanoMay 10, 2022
  7. Glen ChooMay 10, 2022
  8. 0/2 setup.c: make bare repo discovery optionalGlen Choo via GitGitGadget, May 13, 2022
  9. 1/2 setup.c: make bare repo discovery optionalGlen Choo via GitGitGadget, May 13, 2022
  10. Glen ChooMay 16, 2022
  11. Derrick StoleeMay 16, 2022
  12. Taylor BlauMay 16, 2022
  13. Glen ChooMay 17, 2022
  14. Glen ChooMay 17, 2022
  15. 2/2 setup.c: learn discovery.bareRepository=cwdGlen Choo via GitGitGadget, May 13, 2022
  16. Derrick StoleeMay 16, 2022
  17. Junio C HamanoMay 16, 2022
  18. Glen ChooMay 16, 2022
  19. Junio C HamanoMay 16, 2022
  20. Glen ChooMay 16, 2022
  21. Junio C HamanoMay 16, 2022
  22. Junio C HamanoMay 16, 2022
  23. Derrick StoleeMay 16, 2022
  24. Taylor BlauMay 16, 2022
  25. Junio C HamanoMay 16, 2022
  26. Glen ChooMay 17, 2022
  27. 0/5 config: introduce discovery.bare and protected configGlen Choo via GitGitGadget, May 27, 2022
  28. 1/5 Documentation: define protected configurationGlen Choo via GitGitGadget, May 27, 2022
  29. Junio C HamanoMay 27, 2022
  30. Derrick StoleeJun 2, 2022
  31. Junio C HamanoJun 2, 2022
  32. Glen ChooJun 2, 2022
  33. Glen ChooJun 3, 2022
  34. 2/5 config: read protected config with `git_protected_config()`Glen Choo via GitGitGadget, May 27, 2022
  35. Junio C HamanoMay 28, 2022
  36. Glen ChooMay 31, 2022
  37. Junio C HamanoJun 1, 2022
  38. Derrick StoleeJun 2, 2022
  39. 3/5 setup.c: create `discovery.bare`Glen Choo via GitGitGadget, May 27, 2022
  40. Junio C HamanoMay 28, 2022
  41. Derrick StoleeJun 2, 2022
  42. 5/5 upload-pack: make uploadpack.packObjectsHook protectedGlen Choo via GitGitGadget, May 27, 2022
  43. Derrick StoleeJun 2, 2022
  44. 4/5 config: include "-c" in protected configGlen Choo via GitGitGadget, May 27, 2022
  45. Derrick StoleeJun 2, 2022
  46. 0/5 config: introduce discovery.bare and protected configGlen Choo via GitGitGadget, Jun 7, 2022
  47. 4/5 safe.directory: use git_protected_config()Glen Choo via GitGitGadget, Jun 7, 2022
  48. 3/5 config: read protected config with `git_protected_config()`Glen Choo via GitGitGadget, Jun 7, 2022
  49. Junio C HamanoJun 7, 2022
  50. Glen ChooJun 8, 2022
  51. 5/5 setup.c: create `discovery.bare`Glen Choo via GitGitGadget, Jun 7, 2022
  52. Glen ChooJun 7, 2022
  53. 2/5 Documentation: define protected configurationGlen Choo via GitGitGadget, Jun 7, 2022
  54. Jonathan TanJun 22, 2022
  55. Glen ChooJun 23, 2022
  56. 1/5 Documentation/git-config.txt: add SCOPES sectionGlen Choo via GitGitGadget, Jun 7, 2022
  57. Jonathan TanJun 22, 2022
  58. Glen ChooJun 23, 2022
  59. Junio C HamanoJun 23, 2022
  60. Glen ChooJun 27, 2022
  61. Glen ChooJun 27, 2022
  62. 0/5 config: introduce discovery.bare and protected configGlen Choo via GitGitGadget, Jun 27, 2022
  63. 2/5 Documentation: define protected configurationGlen Choo via GitGitGadget, Jun 27, 2022
  64. 4/5 safe.directory: use git_protected_config()Glen Choo via GitGitGadget, Jun 27, 2022
  65. 3/5 config: learn `git_protected_config()`Glen Choo via GitGitGadget, Jun 27, 2022
  66. 5/5 setup.c: create `discovery.bare`Glen Choo via GitGitGadget, Jun 27, 2022
  67. Ævar Arnfjörð BjarmasonJun 30, 2022
  68. Glen ChooJun 30, 2022
  69. 1/5 Documentation/git-config.txt: add SCOPES sectionGlen Choo via GitGitGadget, Jun 27, 2022
  70. 0/5 config: introduce discovery.bare and protected configGlen Choo via GitGitGadget, Jun 30, 2022
  71. 1/5 Documentation/git-config.txt: add SCOPES sectionGlen Choo via GitGitGadget, Jun 30, 2022
  72. Taylor BlauJun 30, 2022
  73. Glen ChooJul 6, 2022
  74. 2/5 Documentation: define protected configurationGlen Choo via GitGitGadget, Jun 30, 2022
  75. Taylor BlauJun 30, 2022
  76. Glen ChooJul 6, 2022
  77. 3/5 config: learn `git_protected_config()`Glen Choo via GitGitGadget, Jun 30, 2022
  78. Taylor BlauJul 1, 2022
  79. Glen ChooJul 6, 2022
  80. 4/5 safe.directory: use git_protected_config()Glen Choo via GitGitGadget, Jun 30, 2022
  81. 5/5 setup.c: create `discovery.bare`Glen Choo via GitGitGadget, Jun 30, 2022
  82. Taylor BlauJul 1, 2022
  83. Glen ChooJul 7, 2022
  84. Taylor BlauJun 30, 2022
  85. Ævar Arnfjörð BjarmasonJun 30, 2022
  86. Glen ChooJul 1, 2022
  87. Ævar Arnfjörð BjarmasonJul 8, 2022
  88. Glen ChooJul 12, 2022
  89. Ævar Arnfjörð BjarmasonJul 12, 2022
  90. 0/5 config: introduce discovery.bare and protected configGlen Choo via GitGitGadget, Jul 7, 2022
  91. 1/5 Documentation/git-config.txt: add SCOPES sectionGlen Choo via GitGitGadget, Jul 7, 2022
  92. Junio C HamanoJul 7, 2022
  93. Glen ChooJul 8, 2022
  94. Junio C HamanoJul 8, 2022
  95. Glen ChooJul 8, 2022
  96. 2/5 Documentation: define protected configurationGlen Choo via GitGitGadget, Jul 7, 2022
  97. Junio C HamanoJul 8, 2022
  98. 3/5 config: learn `git_protected_config()`Glen Choo via GitGitGadget, Jul 7, 2022
  99. 4/5 safe.directory: use git_protected_config()Glen Choo via GitGitGadget, Jul 7, 2022
  100. 5/5 setup.c: create `discovery.bare`Glen Choo via GitGitGadget, Jul 7, 2022
  101. Junio C HamanoJul 8, 2022
  102. Glen ChooJul 8, 2022
  103. Glen ChooJul 12, 2022
  104. 0/5 config: introduce safe.bareRepository and protected configGlen Choo via GitGitGadget, Jul 14, 2022
  105. 1/5 Documentation/git-config.txt: add SCOPES sectionGlen Choo via GitGitGadget, Jul 14, 2022
  106. 2/5 Documentation: define protected configurationGlen Choo via GitGitGadget, Jul 14, 2022
  107. 3/5 config: learn `git_protected_config()`Glen Choo via GitGitGadget, Jul 14, 2022
  108. SANITIZE=address failure on master (was: [PATCH v8 3/5] config: learn `git_protected_config()`)Ævar Arnfjörð Bjarmason, Jul 25, 2022
  109. Glen ChooJul 25, 2022
  110. Ævar Arnfjörð BjarmasonJul 25, 2022
  111. Glen ChooJul 25, 2022
  112. 4/5 safe.directory: use git_protected_config()Glen Choo via GitGitGadget, Jul 14, 2022
  113. 5/5 setup.c: create `safe.bareRepository`Glen Choo via GitGitGadget, Jul 14, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.