| Message ID | cd1e1f6985c77d21ec869e53dc5eb79673caf491.1623343713.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix uninitialised reads found with MSAN | expand |
On Thu, Jun 10, 2021 at 9:49 AM Andrzej Hunt via GitGitGadget <gitgitgadget@gmail.com> wrote: > [snip] Therefore we add a > memset to convince MSAN that this memory is safe to read - but only > when building with MSAN to avoid this cost in normal usage. It does not seem likely to be that expensive, and would definitely be shorter without all the `#if` testing: > diff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c > index 289a9b8f89d0..02fa5285988f 100644 > --- a/builtin/checkout--worker.c > +++ b/builtin/checkout--worker.c > @@ -56,6 +56,17 @@ static void report_result(struct parallel_checkout_item *pc_item) > struct pc_item_result res; This could just have `= { 0 }` added. In any case, this and all the others in this series look good to me. Chris
Chris Torek <chris.torek@gmail.com> writes: > On Thu, Jun 10, 2021 at 9:49 AM Andrzej Hunt via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> [snip] Therefore we add a >> memset to convince MSAN that this memory is safe to read - but only >> when building with MSAN to avoid this cost in normal usage. > > It does not seem likely to be that expensive, and would definitely > be shorter without all the `#if` testing: > >> diff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c >> index 289a9b8f89d0..02fa5285988f 100644 >> --- a/builtin/checkout--worker.c >> +++ b/builtin/checkout--worker.c >> @@ -56,6 +56,17 @@ static void report_result(struct parallel_checkout_item *pc_item) >> struct pc_item_result res; > > This could just have `= { 0 }` added. I'd prefer that very much more than the #if testing, within which // comments that we do not want in our codebase are enclosed. Thanks.
On 11/06/2021 08:28, Junio C Hamano wrote: > Chris Torek <chris.torek@gmail.com> writes: > >> On Thu, Jun 10, 2021 at 9:49 AM Andrzej Hunt via GitGitGadget >> <gitgitgadget@gmail.com> wrote: >>> [snip] Therefore we add a >>> memset to convince MSAN that this memory is safe to read - but only >>> when building with MSAN to avoid this cost in normal usage. >> >> It does not seem likely to be that expensive, and would definitely >> be shorter without all the `#if` testing: >> >>> diff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c >>> index 289a9b8f89d0..02fa5285988f 100644 >>> --- a/builtin/checkout--worker.c >>> +++ b/builtin/checkout--worker.c >>> @@ -56,6 +56,17 @@ static void report_result(struct parallel_checkout_item *pc_item) >>> struct pc_item_result res; >> >> This could just have `= { 0 }` added. > > I'd prefer that very much more than the #if testing, within which // > comments that we do not want in our codebase are enclosed. I'll fix this for V2 - thanks Chris and Junio! (At the time I wasn't aware that this would include all members and padding, but I've learned more since reading the clang developer's discussion around padding and brace intialisation :) : https://reviews.llvm.org/D61280 . )
Andrzej Hunt <andrzej@ahunt.org> writes: > (At the time I wasn't aware that this would include all members and > padding, but I've learned more since reading the clang developer's > discussion around padding and brace intialisation :) : > https://reviews.llvm.org/D61280 . ) Thanks for a pointer ;-)
diff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c index 289a9b8f89d0..02fa5285988f 100644 --- a/builtin/checkout--worker.c +++ b/builtin/checkout--worker.c @@ -56,6 +56,17 @@ static void report_result(struct parallel_checkout_item *pc_item) struct pc_item_result res; size_t size; +#if defined(__has_feature) +# if __has_feature(memory_sanitizer) + // MSAN workaround: res contains padding bytes, which will remain + // permanently unintialised. Later, we read all of res in order to send + // it to the parent process - and MSAN (rightly) complains that we're + // reading those unintialised padding bytes. By memset'ing res we + // guarantee that there are no uninitialised bytes. + memset(&res, 0, sizeof(res)); +#endif +#endif + res.id = pc_item->id; res.status = pc_item->status;