* [bug] possible condition depending on uninitialized value in _notmuch_message_sync
@ 2022-05-16 9:27 Eliza Velasquez
2022-05-16 9:47 ` David Bremner
0 siblings, 1 reply; 6+ messages in thread
From: Eliza Velasquez @ 2022-05-16 9:27 UTC (permalink / raw)
To: notmuch
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug] possible condition depending on uninitialized value in _notmuch_message_sync
2022-05-16 9:27 [bug] possible condition depending on uninitialized value in _notmuch_message_sync Eliza Velasquez
@ 2022-05-16 9:47 ` David Bremner
2022-05-16 11:33 ` Eliza Velasquez
0 siblings, 1 reply; 6+ messages in thread
From: David Bremner @ 2022-05-16 9:47 UTC (permalink / raw)
To: Eliza Velasquez, notmuch
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug] possible condition depending on uninitialized value in _notmuch_message_sync
2022-05-16 9:47 ` David Bremner
@ 2022-05-16 11:33 ` Eliza Velasquez
2022-05-16 12:01 ` David Bremner
2022-05-20 12:46 ` David Bremner
0 siblings, 2 replies; 6+ messages in thread
From: Eliza Velasquez @ 2022-05-16 11:33 UTC (permalink / raw)
To: David Bremner, notmuch
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug] possible condition depending on uninitialized value in _notmuch_message_sync
2022-05-16 11:33 ` Eliza Velasquez
@ 2022-05-16 12:01 ` David Bremner
2022-05-20 12:46 ` David Bremner
1 sibling, 0 replies; 6+ messages in thread
From: David Bremner @ 2022-05-16 12:01 UTC (permalink / raw)
To: Eliza Velasquez, notmuch
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
.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug] possible condition depending on uninitialized value in _notmuch_message_sync
2022-05-16 11:33 ` Eliza Velasquez
2022-05-16 12:01 ` David Bremner
@ 2022-05-20 12:46 ` David Bremner
2022-05-25 1:42 ` Eliza Velasquez
1 sibling, 1 reply; 6+ messages in thread
From: David Bremner @ 2022-05-20 12:46 UTC (permalink / raw)
To: Eliza Velasquez, notmuch
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;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [bug] possible condition depending on uninitialized value in _notmuch_message_sync
2022-05-20 12:46 ` David Bremner
@ 2022-05-25 1:42 ` Eliza Velasquez
0 siblings, 0 replies; 6+ messages in thread
From: Eliza Velasquez @ 2022-05-25 1:42 UTC (permalink / raw)
To: David Bremner, notmuch
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 :)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-25 1:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-16 9:27 [bug] possible condition depending on uninitialized value in _notmuch_message_sync Eliza Velasquez
2022-05-16 9:47 ` David Bremner
2022-05-16 11:33 ` Eliza Velasquez
2022-05-16 12:01 ` David Bremner
2022-05-20 12:46 ` David Bremner
2022-05-25 1:42 ` Eliza Velasquez
Code repositories for project(s) associated with this public inbox
https://yhetil.org/notmuch.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).