{"thread":{"id":"57804","subject":"[PATCH] setup: avoid uninitialized use of is_safe in ensure_valid_ownership","startedAt":"2022-04-27T08:04:32Z","lastAt":"2022-04-27T16:35:20Z","messageCount":5,"participants":["Carlo Marcelo Arenas Belón","Carlo Arenas","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"454487","messageId":"20220427080412.37766-1-carenas@gmail.com","threadId":"57804","inReplyTo":null,"subject":"[PATCH] setup: avoid uninitialized use of is_safe in ensure_valid_ownership","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2022-04-27T08:04:12Z","receivedAt":"2022-04-27T08:04:32Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"8959555cee7 (setup_git_directory(): add an owner check for the top-level\ndirectory, 2022-03-02) adds this member as part of a newly created\nstructure that then gets initialized during the callback, but bb50ec3cc30\n(setup: fix safe.directory key not being checked, 2022-04-13) add a\nquick exit from the callback that avoids this initialization unless the\ncallback is called with the relevant key.\n\nThis leads to this variable not being initialized UNLESS the global config\nhas at least one key for safe.directory, so instead initialize it in the\ncaller.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n setup.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex a7b36f3ffbf..17c7f5fc1dc 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1122,7 +1122,7 @@ static int safe_directory_cb(const char *key, const char *value, void *d)\n \n static int ensure_valid_ownership(const char *path)\n {\n-\tstruct safe_directory_data data = { .path = path };\n+\tstruct safe_directory_data data = { .path = path, .is_safe = 0 };\n \n \tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n \t is_path_owned_by_current_user(path))\n-- \n2.36.0.266.g59f845bde02\n\n"},{"id":"454488","messageId":"CAPUEsphL6TARGuhjvCiXXt+CQZd1+c22KBw5n-XCvnP176yz8Q@mail.gmail.com","threadId":"57804","inReplyTo":"CA+zfrf-6c7BG-PDehHKh6_8zWdu=NeM9gL6zN8Ug+oT9fAOfqw@mail.gmail.com","subject":"Re: [PATCH] setup: avoid uninitialized use of is_safe in ensure_valid_ownership","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2022-04-27T08:31:59Z","receivedAt":"2022-04-27T08:32:16Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Apr 27, 2022 at 1:17 AM Matheus Valadares wrote:\n>\n> That’s not needed. Fields in an aggregate constructor that are not named are initialized to 0.\n\nCould you point me to the C99 specification that documents that if you\nhave it at hand? Even if not strictly necessary, it seems like good\ncode higiene as its effect is hidden.\n\nFWIW, it broke a change I was doing on top of this where I removed the\n\" .path \" initializer to set it later and was suddenly getting random\nunrelated errors because this field was not initialized.\n\nCarlo\n"},{"id":"454489","messageId":"CAPUEspgf_PpjjrGaAqeE50fWYeCqevQRGutocNOA4gyYzQzFsA@mail.gmail.com","threadId":"57804","inReplyTo":"CA+zfrf_mHfDXk-1VhU564YeCTW7rgDeeuORdTT61LkTESmC5Og@mail.gmail.com","subject":"Re: [PATCH] setup: avoid uninitialized use of is_safe in ensure_valid_ownership","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2022-04-27T09:16:17Z","receivedAt":"2022-04-27T09:16:52Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Apr 27, 2022 at 1:33 AM Matheus Valadares wrote:\n>\n> C99 standard §6.7.8 (Initialization)/21,\n>\n> If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration.\n\nThanks, I will include the change to remove the path initializer and\nadd instead the is_safe one as part of my bigger change, but FWIW the\nfollowing doesn't even trigger a warning with the highest level we\nhave with neither a recent clang or gcc or even the cppcheck static\nanalyzer, but leave and uses is_safe uninitialized.\n\ndiff --git a/setup.c b/setup.c\nindex 17c7f5fc1dc..28d008145fa 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1122,7 +1122,9 @@ static int safe_directory_cb(const char *key,\nconst char *value, void *d)\n\n static int ensure_valid_ownership(const char *path)\n {\n- struct safe_directory_data data = { .path = path, .is_safe = 0 };\n+ struct safe_directory_data data;\n+\n+ data.path = path;\n\n if (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n is_path_owned_by_current_user(path))\n\nCarlo\n"},{"id":"454496","messageId":"2ac40f33-8144-e1a1-c3a2-bb9f818287f8@github.com","threadId":"57804","inReplyTo":"CAPUEspgf_PpjjrGaAqeE50fWYeCqevQRGutocNOA4gyYzQzFsA@mail.gmail.com","subject":"Re: [PATCH] setup: avoid uninitialized use of is_safe in ensure_valid_ownership","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-27T14:04:49Z","receivedAt":"2022-04-27T14:04:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/27/2022 5:16 AM, Carlo Arenas wrote:\n> On Wed, Apr 27, 2022 at 1:33 AM Matheus Valadares wrote:\n>>\n>> C99 standard §6.7.8 (Initialization)/21,\n>>\n>> If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration.\n> \n> Thanks, I will include the change to remove the path initializer and\n> add instead the is_safe one as part of my bigger change, but FWIW the\n> following doesn't even trigger a warning with the highest level we\n> have with neither a recent clang or gcc or even the cppcheck static\n> analyzer, but leave and uses is_safe uninitialized.\n> \n> diff --git a/setup.c b/setup.c\n> index 17c7f5fc1dc..28d008145fa 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1122,7 +1122,9 @@ static int safe_directory_cb(const char *key,\n> const char *value, void *d)\n> \n> static int ensure_valid_ownership(const char *path)\n> {\n> - struct safe_directory_data data = { .path = path, .is_safe = 0 };\n\nHere, we are using an initializer, which guarantees that the unmentioned\nmembers are set to zeroes.\n\n> + struct safe_directory_data data;\n\nHere, you are not using an initializer. The data could be anything, so\nis not safe.\n\n> +\n> + data.path = path;\n\nInitializing individual members like this afterwards is not safe unless\nyou set all members individually.\n\nThat is why we use the \"= { ... }\" pattern throughout the codebase.\nSometimes it is simplified to just \"= { 0 }\" to make sure that the first\nmember is mentioned as zero and the remaining members are also set to\nzero as specified in the standard.\n\nThanks,\n-Stolee\n"},{"id":"454508","messageId":"xmqq8rrqo56p.fsf@gitster.g","threadId":"57804","inReplyTo":"20220427080412.37766-1-carenas@gmail.com","subject":"Re: [PATCH] setup: avoid uninitialized use of is_safe in ensure_valid_ownership","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-27T16:35:10Z","receivedAt":"2022-04-27T16:35:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> 8959555cee7 (setup_git_directory(): add an owner check for the top-level\n> directory, 2022-03-02) adds this member as part of a newly created\n> structure that then gets initialized during the callback, but bb50ec3cc30\n> (setup: fix safe.directory key not being checked, 2022-04-13) add a\n> quick exit from the callback that avoids this initialization unless the\n> callback is called with the relevant key.\n>\n> This leads to this variable not being initialized UNLESS the global config\n> has at least one key for safe.directory, so instead initialize it in the\n> caller.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> setup.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/setup.c b/setup.c\n> index a7b36f3ffbf..17c7f5fc1dc 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1122,7 +1122,7 @@ static int safe_directory_cb(const char *key, const char *value, void *d)\n> \n> static int ensure_valid_ownership(const char *path)\n> {\n> -\tstruct safe_directory_data data = { .path = path };\n> +\tstruct safe_directory_data data = { .path = path, .is_safe = 0 };\n\nThis is not wrong per-se but is not necessary. Once you have an\ninitializer, the struct is zero initialized except for members whose\ninitial values are explicitly mentioned in the initializer.\n\nSometimes an explicit initialization is a good way to make the\nintention of the code clear, and because setting of the .is_safe\nmember is done inside a callback function, out of sight from the\nreader of this function, while the return value does depend on\nhaving a valid value in the .is_safe member, I do not think we mind\nthis change, though.\n\nBut if we were to take it, the justification must be rewritten. It\nis an OK change to clarify the code to human readers. It is not a\nfix to a bug that left a struct member uninitialized.\n\n> \n> \tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n> \t is_path_owned_by_current_user(path))\n"}]}