notmuch breaks on \. in config file with upgrade from glib2 2.76.1 to 2.76.5

notmuch breaks on \. in config file with upgrade from glib2 2.76.1 to 2.76.5

[Eric Blake]

Summarizing a regression bug I first reported on IRC, which I hit on
Fedora 38.  These steps reproduce it without impacting anything else
you may have on your system:

$ rpm -q notmuch
notmuch-0.37-5.fc38.x86_64
$ sudo dnf downgrade -y glib2*.x86_64 glib2*.i686
<...>
$ rpm -q glib2
glib2-2.76.1-1.fc38.x86_64
glib2-2.76.1-1.fc38.i686
$ mkdir /tmp/foo
$ cd /tmp/foo
$ cat >.notmuch-config <<EOF
[database]
path=$PWD
EOF
$ yes $'\n' | NOTMUCH_notmuch --config=$PWD/.notmuch-config setup
<...>
$ notmuch --config=$PWD/.notmuch-config new
Found 1 total files (that's not much mail).
Note: Ignoring non-mail file: /tmp/foo/.notmuch-config
Processed 1 file in almost no time.
No new mail.
$ notmuch --config=$PWD/.notmuch-config config set query.q1 'from:/.*\.example\.org/'
$ printf 'q2=from:"/.*\\.example\\.org/"\n' >> .notmuch-config
$ tail -n3 .notmuch-config
[query]
q1=from:/.*\\.example\\.org/
q2=from:"/.*\.example\.org/"
$ notmuch --config=$PWD/.notmuch-config config get query.q1
from:/.*\.example\.org/
$ notmuch --config=$PWD/.notmuch-config config get query.q2
from:"/.*\.example\.org/"
$ notmuch --config=$PWD/.notmuch-config count
0
$ sudo dnf upgrade -y
$ rpm -q glib2
glib2-2.76.5-1.fc38.x86_64
glib2-2.76.5-1.fc38.i686
$ notmuch --config=$PWD/.notmuch-config config list
$ echo $?
0
$ notmuch --config=$PWD/.notmuch-config count
$ echo $?
1

Yikes - any configuration I wrote using 'notmuch config set' is stored
by older glib in a way that newer glib can still read.  But any
configuration that I manually added directly into the config file
might contain data that older glib can parse but newer glib rejects.
In this particular case, it looks like older glib happily treats "\."
as a string with two literal characters, while newer glib is trying to
treat it as an escape sequence and failing.  This failure is then
poorly handled by notmuch, which has knock-on odd effects ('config
list' producing no output but exiting successfully!, 'count' producing
no output but giving no error message why, etc.).

Last night, I filed
https://bugzilla.redhat.com/show_bug.cgi?id=2237562.  Later, I found
this about glib 2.77 introducing regressions:
https://bugzilla.redhat.com/show_bug.cgi?id=2225257; looks like Fedora
backported enough of that into 2.76.5 to cause similar issues in
relation to 2.76.1, even though a patchlevel release of glib shouldn't
be changing behaviors.

I presume that 'notmuch config set' should be the preferred way to
modify the config file - but since it IS a human-readable file,
notmuch should do a much better job of reporting errors whenever
glib's gkeyfile API cannot parse the file (even if that failure to
parse is caused by an unintended regression in glib behavior for
rejecting something it used to accept).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: notmuch breaks on \. in config file with upgrade from glib2 2.76.1 to 2.76.5

[Michael J Gruber]

Hi there

[snip]
> Last night, I filed
> https://bugzilla.redhat.com/show_bug.cgi?id=2237562.  Later, I found
> this about glib 2.77 introducing regressions:
> https://bugzilla.redhat.com/show_bug.cgi?id=2225257; looks like Fedora
> backported enough of that into 2.76.5 to cause similar issues in
> relation to 2.76.1, even though a patchlevel release of glib shouldn't
> be changing behaviors.

Fedora has no related patches in 2.76.5-1 at all (only hmac/eperm).
So, if that's the same regression as in 2.77 it was introduced
earlier, and purely in upstream.

> I presume that 'notmuch config set' should be the preferred way to
> modify the config file - but since it IS a human-readable file,
> notmuch should do a much better job of reporting errors whenever
> glib's gkeyfile API cannot parse the file (even if that failure to
> parse is caused by an unintended regression in glib behavior for
> rejecting something it used to accept).

Yes. This round of glib2 gave us quite some headaches to get config
back working at all. The typical response from glib2 upstream was that
what we called regressions were fixes to wrong behaviour in glib2 and
that we should not rely on established behaviour (my words) but only
on the documentation, the latter exposing a sense of humour which I do
appreciate at times.

In particular, glib2's read and write results are the authoritative
answer. And probably the older glib2 was "wrong" in what it accepted
leniently ... Does notmuch even get the chance to read partially?

Michael

Re: notmuch breaks on \. in config file with upgrade from glib2 2.76.1 to 2.76.5

[Eric Blake]

On Wed, Sep 06, 2023 at 04:37:57PM +0200, Michael J Gruber wrote:
> Hi there
> 
> [snip]
> > Last night, I filed
> > https://bugzilla.redhat.com/show_bug.cgi?id=2237562.  Later, I found
> > this about glib 2.77 introducing regressions:
> > https://bugzilla.redhat.com/show_bug.cgi?id=2225257; looks like Fedora
> > backported enough of that into 2.76.5 to cause similar issues in
> > relation to 2.76.1, even though a patchlevel release of glib shouldn't
> > be changing behaviors.
> 
> Fedora has no related patches in 2.76.5-1 at all (only hmac/eperm).
> So, if that's the same regression as in 2.77 it was introduced
> earlier, and purely in upstream.

In the meantime, I've pinpointed where the problem was introduced in
glib (71b7efd08a on mainline was backported as 3fd1b63453 on glib-2-76
just before 2.76.5 was cut); and they have a patch pending to fix it
which I have now tested:

https://gitlab.gnome.org/GNOME/glib/-/merge_requests/3565

> 
> > I presume that 'notmuch config set' should be the preferred way to
> > modify the config file - but since it IS a human-readable file,
> > notmuch should do a much better job of reporting errors whenever
> > glib's gkeyfile API cannot parse the file (even if that failure to
> > parse is caused by an unintended regression in glib behavior for
> > rejecting something it used to accept).
> 
> Yes. This round of glib2 gave us quite some headaches to get config
> back working at all. The typical response from glib2 upstream was that
> what we called regressions were fixes to wrong behaviour in glib2 and
> that we should not rely on established behaviour (my words) but only
> on the documentation, the latter exposing a sense of humour which I do
> appreciate at times.
> 
> In particular, glib2's read and write results are the authoritative
> answer. And probably the older glib2 was "wrong" in what it accepted
> leniently ... Does notmuch even get the chance to read partially?

GError are intended to be used as recoverable errors - we have every
right to refactor the logic to ignore keys that are unreadable while
still moving forward with the rest of the file, if we want to do
partial reads.  But that's more invasive than the patch I just
proposed, which merely prints the GError message to the user (serves
as a warning to a user that their hand-written invalid escapes are
being accepted anyways with 2.76.1, and gives the user an explanation
why notmuch isn't working with 2.76.5).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

[PATCH] config: Inform user if config file is broken

[Eric Blake]

glib 2.76.1 silently treats invalid escape sequences as two
characters, even though it is willing to set a GError warning about
it.  While 'notmuch config set' never produces such sequences in the
config file, the fact that the config file is human-readable lends
itself to hand-written edits, where the person making the edits can
introduce things such as:

[query]
foo = from:/example\.org/

instead of the correct

[query]
foo = from:/example\\.org/

glib 2.76.5 turned this into a hard error, but nothing higher in the
call stack outputs anything to the user in the case of
NOTMUCH_STATUS_FILE_ERROR to let the user know the problem ('notmuch
new' and 'notmuch count' silently fail with no output; 'notmuch config
list' outputs nothing and reports success).  While glib will be fixing
their regression before 2.78 [1], it is likely that future glib will
restore the hard error.  Thus, we should inform the user any time
their config file cannot be parsed; this gives a warning when using
glib 2.76.1 where the parse is still successful, and gives an
explanation why nothing happens during 'notmuch count' or 'notmuch
config list' when using glib 2.76.5 where the parse fails.

The added output message may still not be the most obvious:

$ notmuch --config=$PWD/.notmuch-config count
Key file contains key “foo” which has a value that cannot be interpreted.

but it is better than silence.

[1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/3565

Fixes: https://nmbug.notmuchmail.org/nmweb/show/5a7paaqa2dvdo5lmnxvaeacfwhdytfnkr4gfh6mtlotdviki2s%40ro4gz4m2aqsw
---

I'm not sure if this is the best approach (as this is my first ever
patch to notmuch), but it's better than nothing.

[And if Carl Worth still reads the list - thanks for introducing me to
emacs back in 2000 when we worked together as students at BYU]

 lib/config.cc | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/config.cc b/lib/config.cc
index 2323860d..afe8f429 100644
--- a/lib/config.cc
+++ b/lib/config.cc
@@ -435,6 +435,7 @@ _notmuch_config_load_from_file (notmuch_database_t *notmuch,
 	for (gchar **keys_p = keys; *keys_p; keys_p++) {
 	    char *absolute_key = talloc_asprintf (notmuch, "%s.%s", *grp,  *keys_p);
 	    char *normalized_val;
+	    GError *gerr = NULL;

 	    /* If we opened from a given path, do not overwrite it */
 	    if (strcmp (absolute_key, "database.path") == 0 &&
@@ -442,7 +443,11 @@ _notmuch_config_load_from_file (notmuch_database_t *notmuch,
 		notmuch->xapian_db)
 		continue;

-	    val = g_key_file_get_string (file, *grp, *keys_p, NULL);
+	    val = g_key_file_get_string (file, *grp, *keys_p, &gerr);
+	    if (gerr) {
+		fprintf (stderr, "%s\n", gerr->message);
+		g_error_free (gerr);
+	    }
 	    if (! val) {
 		status = NOTMUCH_STATUS_FILE_ERROR;
 		goto DONE;

base-commit: 5303e35089e1a8ffcdb1d5891bc85d3f6c401a8f
prerequisite-patch-id: e81473c9dc7ffd8b3b9bf64b3f3edb84bfb99bbb
-- 
2.41.0
\r

Re: [PATCH] config: Inform user if config file is broken

[David Bremner]

Eric Blake <eblake@redhat.com> writes:

>
> I'm not sure if this is the best approach (as this is my first ever
> patch to notmuch), but it's better than nothing.

Unfortunately we can't just print from there because it is in a shared
library (whose clients might not appreciate output).  Something _almost_
equivalent can be done with _notmuch_database_log, but that still
requires the caller to read those logs with
notmuch_database_status_string.

d

Re: [PATCH] config: Inform user if config file is broken

[Eric Blake]

On Wed, Sep 06, 2023 at 12:48:15PM -0300, David Bremner wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
> >
> > I'm not sure if this is the best approach (as this is my first ever
> > patch to notmuch), but it's better than nothing.
> 
> Unfortunately we can't just print from there because it is in a shared
> library (whose clients might not appreciate output).  Something _almost_
> equivalent can be done with _notmuch_database_log, but that still
> requires the caller to read those logs with
> notmuch_database_status_string.

I'm out of time to spend further on this bug today; if you would like
to take the ideas in my patch and rework it into something usable, be
my guest.  Otherwise, I might be able to return to this bug later in
the week to see if I can figure out how to grab the database_log at
the right point when status is NOTMUCH_STATUS_FILE_ERROR is returned
(open.cc:notmuch_database_load_config DONE label looks like it should
be able to grab from the database log if status_string is present).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: notmuch breaks on \. in config file with upgrade from glib2 2.76.1 to 2.76.5

[David Bremner]

Eric Blake <eblake@redhat.com> writes:
> $ tail -n3 .notmuch-config
> [query]
> q1=from:/.*\\.example\\.org/
> q2=from:"/.*\.example\.org/"

> glib2-2.76.5-1.fc38.x86_64
> glib2-2.76.5-1.fc38.i686
> $ notmuch --config=$PWD/.notmuch-config config list
> $ echo $?
> 0
> $ notmuch --config=$PWD/.notmuch-config count
> $ echo $?
> 1

I could not duplicate the glib issue with glib 2.77.2 on Debian.
Looking at the source, it looks like we don't have 71b7efd08a1fe
(or equivalent).

╰─ (git)-[release]-% tail -3 bad-config
[query]
q1=from:/.*\\.example\\.org/
q2=from:"/.*\.example\.org/"
╭─ motzkin:upstream/notmuch/test/tmp.T030-config
╰─ (git)-[release]-% ../../notmuch --config=./bad-config count
52
╭─ motzkin:upstream/notmuch/test/tmp.T030-config
╰─ (git)-[release]-% echo $?
0

I usually like to start with a failing test, but it seems that may not
be possible here, since the actual failure only happens with specific
versions of glib.

From the discussion surrounding the revert / fix, it seems like the
behaviour of setting the GError, but also returning a string is itself
an error and can be expected to change. The assumption in the notmuch
code [1] is also that the status string only needs to be checked
on non-success status code. It should be OK to check the string anyway, but I
guess there might be some bugs uncovered.



[1]: notmuch.c, around line 547\r

Re: notmuch breaks on \. in config file with upgrade from glib2 2.76.1 to 2.76.5

[David Bremner]

David Bremner <david@tethera.net> writes:

> I usually like to start with a failing test, but it seems that may not
> be possible here, since the actual failure only happens with specific
> versions of glib.

I guess the more interesting issue is making sure we propagate parsing
errors from glib properly. Do you have an example of an init file syntax
error that does consistently trigger a NULL return value (i.e. not bad
escape, since that is apparently in flux).

d

Re: notmuch breaks on \. in config file with upgrade from glib2 2.76.1 to 2.76.5

[Eric Blake]

On Sun, Sep 10, 2023 at 08:58:39AM -0300, David Bremner wrote:
> David Bremner <david@tethera.net> writes:
> 
> > I usually like to start with a failing test, but it seems that may not
> > be possible here, since the actual failure only happens with specific
> > versions of glib.
>
> I guess the more interesting issue is making sure we propagate parsing
> errors from glib properly. Do you have an example of an init file syntax
> error that does consistently trigger a NULL return value (i.e. not bad
> escape, since that is apparently in flux).

Sure - put in some invalid UTF-8 (unlike invalid escape sequences
changing behavior over time, invalid UTF-8 has always been invalid in
g_key_file_get_string).  For example,

printf 'q3=from:\xff\n' >> .notmuch-config

into a config file that previously contains valid UTF-8 and ends in a
[query] block.

Even with glib2-2.76.1 installed, I'm back to the scenario where
'notmuch --config=.notmuch-config config list' outputs nothing with
exit status 0 (when it SHOULD have been reporting glib's error, "Key
file contains key “%s” with value “%s” which is not UTF-8").

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org
\r