[BUG] Structured logging must pass GLIB_DOMAIN as nul-terminated string

[swsnr]

Bug description

Currently, log_structured! passes GLIB_DOMAIN as a log field with byte value and explicit length. However, GLib expects GLIB_DOMAIN to be a nul-terminated C string, see should_drop_message which calls domain_found which in turn uses strlen on the domain value.

This breaks debug logging for Rust domains via $G_MESSAGES_DEBUG, because GLib fails to find the domain in the environment variable, see #1534

I'm only superficially familiar with C, and haven't checked the entire call stack, but it also seems that passing GLIB_DOMAIN as field with explicit length, i.e. omitting the final NUL byte from the field value, probably even leads to out-of-bounds memory access in GLib 🤔

I'm not sure whether this constitutes an issue in GLib, but it can be fixed on the Rust side by passing the domain as nul-terminated string, at the cost of one allocation. I'll make a pull request.

[swsnr]

I've opened #1539 with a fix.

[pwithnall]

The GLib documentation says what the expected behaviour is fairly clearly:

Note that keys that expect UTF-8 strings (specifically "MESSAGE" and "GLIB_DOMAIN") must be passed as nul-terminated UTF-8 strings until GLib version 2.74.1 because the default log handler did not consider the length of the GLogField. Starting with GLib 2.74.1 this is fixed and non-nul-terminated UTF-8 strings can be passed with their correct length.

So if this is something you’re experiencing with GLib > 2.74.1, then it’s a bug in GLib and it would be great if you could file it upstream.

[swsnr]

@pwithnall I'm sorry, but this paragraph isn't quite clear to me, as an outsider: For instance, what's the "default log handler", from a Rust point of view? And how does it relate to $G_MESSAGES_DEBUG which is how I came across this (see discussion #1534)?

Now, I'm on Glib 2.82, so presumably this is not intended behaviour, but what exactly should I report? I'm not familiar with C, so I can't even come up with a minimal C example to demonstrate this behaviour. So all I can do is open a Glib ticket and reference this one here; would that be sufficient?

And that said, these Rust bindings appear to maintain compatibility with Glib back to 2.50 (according to the features list in glib/Cargo.toml), so whether Glib behaves correctly or not is a moot question. This crate needs to be fixed in any case.

[sdroege]

Now, I'm on Glib 2.82, so presumably this is not intended behaviour, but what exactly should I report? I'm not familiar with C, so I can't even come up with a minimal C example to demonstrate this behaviour. So all I can do is open a Glib ticket and reference this one here; would that be sufficient?

Yes this shouldn't happen with 2.82. I'll take a look at this some time in the next days.

whether Glib behaves correctly or not is a moot question. This crate needs to be fixed in any case.

It's a bug in GLib so that's also not very clear. I thought I dropped < 2.74.1 support for structured logging when I noticed this but apparently not. I'll do that in the next days too then, thanks for noticing :)

[swsnr]

@sdroege Thanks. So I understand you already have this on your radar and I don't need to open a separate glib ticket now?

But even if glib is fixed, shouldn't this be fixed here anyway? It could take a long time for glib releases to tickle down through distributions and flatpak runtime after all 🤔

[sdroege]

Yes, I'll take care of this. Don't worry :)

We can try to work around problems with older GLib versions if necessary and possible in a reasonable way but by using outdated software you also get lots of other bugs, including memory safety bugs, so overall this seems a bit futile.

[swsnr]

I guess that's a difference in philosophy, but I don't mind either way. It's just for debug logging anyway, and developers probably tend to use quite recent glib versions anyway 🙂

[swsnr]

Should I close this issue then?

[sdroege]

I guess that's a difference in philosophy

I'm aware of a couple of bugs that were fixed since 2.50 that allow you to crash your application in all kinds of interesting ways. I think the only solution around that would be if we bundle latest GLib with the bindings, but that causes all kinds of other issues.

By using old versions of software, people did a (hopefully conscious) decision to give higher priority to a feeling of "stability" over having known bugs fixed. Or got locked into older versions by some company (hello NVIDIA L4T users).

Should I close this issue then?

No, it's my reminder to look through this in detail :)

[sdroege]

Yes it's indeed a bug in GLib, see https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4350. I can confirm that it works correctly after that change in your example. Also fixed a similar but less critical issue while at it. It looks like all other usage in that file is correct, fortunately.