[bug] possible condition depending on uninitialized value in _notmuch_message_sync

[bug] possible condition depending on uninitialized value in _notmuch_message_sync

[Eliza Velasquez]

Hello notmuch,

I noticed something peculiar while hacking on the notmuch Rust bindings.
One of the unit tests, when run through valgrind, consistently produced
this trace:

--8<---------------cut here---------------start------------->8---
==232461== Thread 2 test_tags::mutab:
==232461== Conditional jump or move depends on uninitialised value(s)
==232461==    at 0x486E8C6: _notmuch_message_sync (in /nix/store/w5i4pvirysllyh6wq5pxqcm62j4g36fl-notmuch-0.35/lib/libnotmuch.so.5.6.0)
==232461==    by 0x4870E6E: notmuch_message_remove_tag (in /nix/store/w5i4pvirysllyh6wq5pxqcm62j4g36fl-notmuch-0.35/lib/libnotmuch.so.5.6.0)
==232461==    by 0x1BE953: notmuch::message::Message::remove_tag (message.rs:125)
==232461==    by 0x15FD83: tests::test_tags::mutable::test_discard_not_present (test_tags.rs:118)
==232461==    by 0x159EC9: tests::test_tags::mutable::test_discard_not_present::{{closure}} (test_tags.rs:114)
==232461==    by 0x14FCBD: core::ops::function::FnOnce::call_once (function.rs:227)
==232461==    by 0x171B52: test::__rust_begin_short_backtrace (in /home/eliza/Git/notmuch-rs/target/debug/deps/tests-36359cb32ba337d6)
==232461==    by 0x171CD8: test::run_test_in_process (in /home/eliza/Git/notmuch-rs/target/debug/deps/tests-36359cb32ba337d6)
==232461==    by 0x19E2B7: _ZN3std10sys_common9backtrace28__rust_begin_short_backtrace17hd93c8f36d9bfcd36E.llvm.6931861783653830752 (in /home/eliza/Git/notmuch-rs/target/debug/deps/tests-36359cb32ba337d6)
==232461==    by 0x184605: core::ops::function::FnOnce::call_once{{vtable.shim}} (in /home/eliza/Git/notmuch-rs/target/debug/deps/tests-36359cb32ba337d6)
==232461==    by 0x2896E2: std::sys::unix::thread::Thread::new::thread_start (in /home/eliza/Git/notmuch-rs/target/debug/deps/tests-36359cb32ba337d6)
==232461==    by 0x4D9CEB1: start_thread (in /nix/store/ayrsyv7npr0lcbann4k9lxr19x813f0z-glibc-2.34-115/lib/libc.so.6)
--8<---------------cut here---------------end--------------->8---

The test definition follows.

--8<---------------cut here---------------start------------->8---
#[test]
fn test_discard_not_present() {
    let tagset = TagSetFixture::new(true, false);
    assert!(!tagset.message.tags().any(|x| x == "foo"));

    tagset.message.remove_tag("foo").unwrap();
}
--8<---------------cut here---------------end--------------->8---

First line, when called with these two arguments, creates a new notmuch
database with a single dummy message and opens it in `ReadWrite' mode.
Investigating the rest of the Rust-side of the stack trace, I didn't
find anything particularly suspicious in the way it handles memory.

Is it possible then that there's a potential memory error with removing
a non-existent tag on a message? I wanted to ask about this on the
mailing list before diving in deeper, since this isn't quite the latest
version of notmuch and I wasn't sure if it had been fixed in 0.36. I
searched the mailing list archives for this particular issue, but I
wasn't able to find anything.

-- 
Eliza

Re: [bug] possible condition depending on uninitialized value in _notmuch_message_sync

[David Bremner]

Eliza Velasquez <eliza@eliza.sh> writes:


> Is it possible then that there's a potential memory error with removing
> a non-existent tag on a message? I wanted to ask about this on the
> mailing list before diving in deeper, since this isn't quite the latest
> version of notmuch and I wasn't sure if it had been fixed in 0.36. I
> searched the mailing list archives for this particular issue, but I
> wasn't able to find anything.

Ideally of course I'd like a reproducer in C.  It would help to have
line numbers in the valgrind output. It might be enough you install the
notmuch debug symbols?

d

Re: [bug] possible condition depending on uninitialized value in _notmuch_message_sync

[Eliza Velasquez]

On Mon, May 16 2022 at 06:47 -03, David Bremner <david@tethera.net> wrote:

> Ideally of course I'd like a reproducer in C.  It would help to have
> line numbers in the valgrind output. It might be enough you install the
> notmuch debug symbols?

Took me a while to figure out the debugging workflow in NixOS, but I
managed to capture the line numbers. At messsage.cc:1333, at the second
condition below:

--8<---------------cut here---------------start------------->8---
/* Synchronize changes made to message->doc out into the database. */
void
_notmuch_message_sync (notmuch_message_t *message)
{
    if (_notmuch_database_mode (message->notmuch) == NOTMUCH_DATABASE_MODE_READ_ONLY)
	return;

    if (! message->modified)
	return;

    ...
}
--8<---------------cut here---------------end--------------->8---

It becomes very clear why this error only happens when removing a
non-existent tag if you look at at message.cc:1570...

--8<---------------cut here---------------start------------->8---
    try {
	message->doc.remove_term (term);
	message->modified = true;
    } catch (const Xapian::InvalidArgumentError) {
	/* We'll let the philosophers try to wrestle with the
	 * question of whether failing to remove that which was not
	 * there in the first place is failure. For us, we'll silently
	 * consider it all good. */
    }
--8<---------------cut here---------------end--------------->8---

So I guess `message->modified' isn't correctly initialized, at least
according to valgrind.

-- 
Eliza

Re: [bug] possible condition depending on uninitialized value in _notmuch_message_sync

[David Bremner]

Eliza Velasquez <eliza@eliza.sh> writes:


> It becomes very clear why this error only happens when removing a
> non-existent tag if you look at at message.cc:1570...
>
> --8<---------------cut here---------------start------------->8---
>     try {
> 	message->doc.remove_term (term);
> 	message->modified = true;
>     } catch (const Xapian::InvalidArgumentError) {
> 	/* We'll let the philosophers try to wrestle with the
> 	 * question of whether failing to remove that which was not
> 	 * there in the first place is failure. For us, we'll silently
> 	 * consider it all good. */
>     }
> --8<---------------cut here---------------end--------------->8---

OK, I see why that assignment gets skipped. I think that's not the
actual bug, but rather message->modified should be initialized in
_notmuch_message_create_for_document. I'll have a closer look later
today

d
.

Re: [bug] possible condition depending on uninitialized value in _notmuch_message_sync

[David Bremner]

Eliza Velasquez <eliza@eliza.sh> writes:

> On Mon, May 16 2022 at 06:47 -03, David Bremner <david@tethera.net> wrote:
>
>> Ideally of course I'd like a reproducer in C.  It would help to have
>> line numbers in the valgrind output. It might be enough you install the
>> notmuch debug symbols?
>
> Took me a while to figure out the debugging workflow in NixOS, but I
> managed to capture the line numbers. At messsage.cc:1333, at the second
> condition below:
>
[snip]
> So I guess `message->modified' isn't correctly initialized, at least
> according to valgrind.
>
> -- 
> Eliza

Can you see if the following change quiets valgrind?

diff --git a/lib/message.cc b/lib/message.cc
index 63b216b6..bd3cb5af 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -169,6 +169,7 @@ _notmuch_message_create_for_document (const void *talloc_owner,
 
     message->doc = doc;
     message->termpos = 0;
+    message->modified = FALSE;
 
     return message;
 }

Re: [bug] possible condition depending on uninitialized value in _notmuch_message_sync

[Eliza Velasquez]

On Fri, May 20 2022 at 09:46 -03, David Bremner <david@tethera.net> wrote:

> Eliza Velasquez <eliza@eliza.sh> writes:
>
>> On Mon, May 16 2022 at 06:47 -03, David Bremner <david@tethera.net> wrote:
>>
>>> Ideally of course I'd like a reproducer in C.  It would help to have
>>> line numbers in the valgrind output. It might be enough you install the
>>> notmuch debug symbols?
>>
>> Took me a while to figure out the debugging workflow in NixOS, but I
>> managed to capture the line numbers. At messsage.cc:1333, at the second
>> condition below:
>>
> [snip]
>> So I guess `message->modified' isn't correctly initialized, at least
>> according to valgrind.
>>
>> -- 
>> Eliza
>
> Can you see if the following change quiets valgrind?
>
> diff --git a/lib/message.cc b/lib/message.cc
> index 63b216b6..bd3cb5af 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -169,6 +169,7 @@ _notmuch_message_create_for_document (const void *talloc_owner,
>  
>      message->doc = doc;
>      message->termpos = 0;
> +    message->modified = FALSE;
>  
>      return message;
>  }

That seems to have fixed it. Valgrind is very pleased with all of the
test cases now :)