For Bug #101354 I found myself needing to copy an arbitrary blob of metadata, and copy it into arbitrarily many subsequent messages. This would be easy in GDBus (we'd store a GVariant reference) but libdbus message items don't have a type, so we have to copy the item into a buffer instead. (I don't want to just take a ref to the message, because the message can contain file descriptors, so keeping it around indefinitely is a DoS.)
Created attachment 132161 [details] [review] message: Add DBusVariant, a way to copy a single message item For #100344, we will need a way to store the metadata from the original method call, and copy them back into arbitrarily many messages later. This would be easy in GDBus, which has GVariant as a first-class object. However, libdbus doesn't have an object for message items, only messages. We could copy the message's content, but it will carry file descriptors, which we don't want to copy. Instead, introduce an internal object representing a message item in a small buffer. It is stored as a variant (D-Bus type 'v') so that it naturally carries its own type.
Created attachment 132162 [details] [review] test-variant: Add a regression test for DBusVariant
Comment on attachment 132161 [details] [review] message: Add DBusVariant, a way to copy a single message item Review of attachment 132161 [details] [review]: ----------------------------------------------------------------- Looks reasonable to me, although I am less familiar with the reader/writer/iter API of libdbus than other bits. Some unit tests would be nice, but I imagine these code paths are tested fairly thoroughly as a result of their use in bug #101354. ::: dbus/dbus-message.c @@ +5215,5 @@ > + goto oom; > + > + if (!_dbus_type_writer_unrecurse (&variant_writer, &inner_writer)) > + goto oom; > + } else { g_assert_not_reached (); /* or equivalent */ } ?
(In reply to Philip Withnall from comment #3) > Some unit tests would be nice They are the other commit here.
Comment on attachment 132162 [details] [review] test-variant: Add a regression test for DBusVariant Review of attachment 132162 [details] [review]: ----------------------------------------------------------------- Oh hey, there’s a unit test in this patch! \o/ Ignore my comments about unit tests in the previous review. ::: test/internals/variant.c @@ +246,5 @@ > + DBUS_TYPE_STRING); > + dbus_message_iter_get_basic (item_iter, &value); > + g_assert_cmpstr (value, ==, expected_value); > + > + v = _dbus_variant_read (item_iter); I think v is leaked? @@ +344,5 @@ > + * int32 42, > + * "Hello, world!", > + * int64 23, > + * [int32 42, int32 42], > + * (int32 42, "Hello, world!", int64 23], s/]/)/
Comment on attachment 132161 [details] [review] message: Add DBusVariant, a way to copy a single message item Review of attachment 132161 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-message.c @@ +5215,5 @@ > + goto oom; > + > + if (!_dbus_type_writer_unrecurse (&variant_writer, &inner_writer)) > + goto oom; > + } Good point.
Created attachment 132412 [details] [review] message: Add DBusVariant, a way to copy a single message item For #100344, we will need a way to store the metadata from the original method call, and copy them back into arbitrarily many messages later. This would be easy in GDBus, which has GVariant as a first-class object. However, libdbus doesn't have an object for message items, only messages. We could copy the message's content, but it will carry file descriptors, which we don't want to copy. Instead, introduce an internal object representing a message item in a small buffer. It is stored as a variant (D-Bus type 'v') so that it naturally carries its own type. --- Use _dbus_string_copy_len() instead of reinventing it, for better range-checking. Move the basic type case to the end, and assert that the type is in fact basic.
Created attachment 132413 [details] [review] internals: Add a log severity that looks like FATAL, but does not _exit() This lets _dbus_warn() and _dbus_warn_check_failed() fall through to flushing stderr and calling _dbus_abort(), meaning that failed checks and warnings can result in a core dump as intended. --- I hit a check failure (in existing code) while running a revised test that exercised more OOM code paths, and was annoyed that the test exited 1 instead of aborting. As an alternative to this we could rename the FATAL severity to ERROR, and change all callers that do not _dbus_abort() (I think there is only one in bus/bus.c, plus one in a test) to exit 1 explicitly.
Created attachment 132414 [details] [review] _dbus_message_set_signature: Delete unused function If this is reinstated it will need some checks. In particular, it was using _dbus_check_is_valid_signature() in an unsafe way: _dbus_check_is_valid_signature() cannot be used in a _dbus_return_val_if_fail() check because it does not distinguish between error by the caller, and out-of-memory conditions.
Created attachment 132415 [details] [review] DBusMessage: Stop using _dbus_check_is_valid_signature() This function looks appealing, but it is a trap, particularly in _dbus_return_val_if_fail() checks. It returns a boolean result, which cannot distinguish between "failed because we ran out of memory" and "failed because the string is actually invalid"; but _dbus_validate_signature_with_reason() allocates memory. Use the over-complicated version directly, so libdbus can continue to bend over backwards to support the (possibly mythical) operating systems that limit memory consumption and do not overcommit, such that malloc() can genuinely return NULL. Bug detected by running the DBusVariant unit test under dbus' failing-malloc() instrumentation.
Created attachment 132416 [details] [review] Remove now-unused _dbus_check_is_valid_signature() As noted in the previous commit, it's a trap.
Created attachment 132417 [details] [review] Remove now-unused _dbus_validate_signature() All callers should use _dbus_validate_signature_with_reason() directly. The only remaining callers were this function's own tests. As a side benefit, this commit removes a TODO pointing out that this function did not follow normal DBusString conventions, by considering a length outside the bounds of the DBusString to be an ordinary lack of validity rather than a fatal programming error.
Created attachment 132418 [details] [review] test-variant: Add a regression test for DBusVariant --- In addition to fixing the issues that Philip pointed out, this now uses _dbus_test_oom_handling() to check the out-of-memory code paths.
There is a memory leak somewhere which I still need to track down.
Comment on attachment 132412 [details] [review] message: Add DBusVariant, a way to copy a single message item Review of attachment 132412 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 132413 [details] [review] internals: Add a log severity that looks like FATAL, but does not _exit() Review of attachment 132413 [details] [review]: ----------------------------------------------------------------- ++ with a commenting/naming nitpick. ::: dbus/dbus-sysdeps-unix.c @@ +4639,4 @@ > case DBUS_SYSTEM_LOG_SECURITY: > flags = LOG_AUTH | LOG_NOTICE; > break; > + case DBUS_SYSTEM_LOG_GOING_TO_BE_FATAL: Maybe it would be worthwhile adding a comment above the exit(1) at the bottom of this function, saying that it’s the caller’s responsibility to abort() on DBUS_SYSTEM_LOG_GOING_TO_BE_FATAL, hence it’s not included in the exit(1) case? Same in dbus-sysdeps-win.c. Or maybe you could rename s/LOG_GOING_TO_BE_FATAL/LOG_FATAL_ABORT_INSTEAD_OF_EXIT/ or something like that, to make it clear from the enum member name rather than needing a comment.
Comment on attachment 132414 [details] [review] _dbus_message_set_signature: Delete unused function Review of attachment 132414 [details] [review]: ----------------------------------------------------------------- Trivially yes. ++
Comment on attachment 132415 [details] [review] DBusMessage: Stop using _dbus_check_is_valid_signature() Review of attachment 132415 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 132416 [details] [review] Remove now-unused _dbus_check_is_valid_signature() Review of attachment 132416 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 132417 [details] [review] Remove now-unused _dbus_validate_signature() Review of attachment 132417 [details] [review]: ----------------------------------------------------------------- ++, modulo one open TODO. ::: dbus/dbus-marshal-validate-util.c @@ +456,5 @@ > + validity = _dbus_validate_signature_with_reason (&str, 0, > + _dbus_string_get_length (&str)); > + > + /* Validity values less than DBUS_VALID are OOM or unknown validity. > + * TODO: specify in which way each one should be invalid */ Are you intending to deal with this TODO in this commit, or leave it for a future patch? Either is fine, just thought I should point it out.
Comment on attachment 132418 [details] [review] test-variant: Add a regression test for DBusVariant Review of attachment 132418 [details] [review]: ----------------------------------------------------------------- ++
This has proved to be a remarkably hirsute yak, which does not fill me with confidence about how well we deal with malloc() returning NULL, despite the heroic efforts taken in libdbus to cope with that. Back in 2008, Havoc wrote <https://blog.ometer.com/2008/02/04/out-of-memory-handling-d-bus-experience/>: There’s also a historical note; I wrote a lot of the code thinking OOM was handled, then later I added testing of most OOM codepaths (with a hack to fail each malloc, running the code over and over). I would guess that when I first added the tests, at least 5% of mallocs were handled in a buggy way ... and guess what? 9 years later we *still* have OOM code paths that are wrong, in code as basic as building a DBusMessage. :-/ In an attempt to make this better, so that Bug #100344 isn't a huge crash risk, I've implemented an API similar to Bug #89104 for DBusMessageIter. I was going to do Bug #89104 too, but ran into the fact that Bug #65959 seems to have been just papering over the symptoms of some deeper bugs... so let's go one step at a time.
(In reply to Philip Withnall from comment #20) > Are you intending to deal with this TODO in this commit, or leave it for a > future patch? Either is fine, just thought I should point it out. A future patch, which I'll attach shortly.
Comment on attachment 132414 [details] [review] _dbus_message_set_signature: Delete unused function Applied
Comment on attachment 132415 [details] [review] DBusMessage: Stop using _dbus_check_is_valid_signature() Applied
Comment on attachment 132416 [details] [review] Remove now-unused _dbus_check_is_valid_signature() Applied
Comment on attachment 132417 [details] [review] Remove now-unused _dbus_validate_signature() Applied, thanks for the reviews so far!
Created attachment 132427 [details] [review] [01] _dbus_marshal_validate_test: Merge two sets of signature validity checks The deleted lines used to be a test for _dbus_validate_signature(), until I deleted that function. We also had a completely separate test for _dbus_validate_signature_with_reason() which remains present. Some of the test vectors were tested in both places. --- This addresses the TODO that Philip noted.
Created attachment 132428 [details] [review] [02] _dbus_marshal_validate_test: Uncomment commented-out test coverage This was added around 12½ years ago, in a commented-out state, and has remained commented out ever since. It turns out these test vectors do pass, although perhaps they didn't at the time. --- I was pleasantly surprised to find out that they do in fact pass.
Created attachment 132429 [details] [review] [03] dbus_message_append_args_valist: Don't leak memory on inappropriate type Found by source code inspection while trying to debug an unrelated leak. --- No unit test for this one, because life's too short to write unit tests for programmer error.
Created attachment 132430 [details] [review] [04] _dbus_message_iter_open_signature: Clarify why this is not leaky The same assertion appears closer to the top of the function, and there is no opportunity for it to have become false here.
Created attachment 132431 [details] [review] [05] dbus_message_iter_append_basic: Don't leak signature if appending fd fails --- Test to follow.
Created attachment 132432 [details] [review] [06] dbus_message_iter_open_container: Don't leak signature on failure If we run out of memory while calling _dbus_type_writer_recurse() (which is impossible for most contained types, but can happen for structs and dict-entries), then the memory we allocated in the call to _dbus_message_iter_open_signature() will still be allocated, and we have to free it in order to return to the state of the world prior to calling open_container(). One might reasonably worry that this change can break callers that use this (incorrect) pattern: if (!dbus_message_iter_open_container (outer, ..., inner)) { dbus_message_iter_abandon_container (outer, inner); goto fail; } /* now we know inner is open, and we must close it later */ However, testing that pattern with _dbus_test_oom_handling() demonstrates that it already dies with a DBusString assertion failure even before this commit. This is all concerningly fragile, and I think the next step should be to zero out DBusMessageIter instances when they are invalidated, so that a "double-free" is always detected.
Created attachment 132433 [details] [review] [07] internals: Make a minimal _dbus_test_oom_handling() universally available Previously, it was only available under DBUS_ENABLE_EMBEDDED_TESTS, because the infrastructure to pretend malloc had failed is only compiled then. However, I'd like to use it in more modular tests, to avoid test-dbus continuing to grow. To facilitate that, inline a trivial version of it when DBUS_ENABLE_EMBEDDED_TESTS is disabled: it just calls the function, once, without doing any strange things to the malloc interface. Similarly, amend the stub implementation of _dbus_get_malloc_blocks_outstanding() so that references to it are syntactically valid, and move the DBusTestMemoryFunction typedef so that it can be used with or without DBUS_ENABLE_EMBEDDED_TESTS.
Created attachment 132434 [details] [review] [08] test/message: Add a targeted test for recently-fixed leaks --- This fails if either of Attachment #132431 [details], Attachment #132432 [details] is reverted. This is my new experiment in combining the OOM-testing code paths from the embedded tests with the conveniently-small, TAP-supporting unit tests from the GLib test stuff. A side benefit is that GLib data structures are immune to dbus_shutdown() and the _dbus_get_malloc_blocks_outstanding() check.
Created attachment 132435 [details] [review] [09] DBusMessageIter: Clarify the API Having opened a container for appending, the container must be closed exactly once.
Created attachment 132436 [details] [review] [10] DBusMessageIter: Zero out the iterator on failure This ensures that callers won't accidentally use it for something in a way that is considered to be programmer error. In _dbus_message_iter_check(), insert a specific check for this before dereferencing iter->message, so that we get a nice assertion failure (potentially non-fatal) instead of a segfault.
Created attachment 132437 [details] [review] [11] DBusMessageIter: Add a function to abandon possibly-zero-filled iterators See the doc-comment of the new dbus_message_iter_abandon_container_if_open() function for details. --- This is basically Bug #89104, but for iterators.
Created attachment 132438 [details] [review] [12] Test dbus_message_iter_abandon_container_if_open under OOM conditions
Created attachment 132439 [details] [review] [13] message: Add DBusVariant, a way to copy a single message item --- Free the contained_signature. This is the memory leak to which I alluded yesterday.
Created attachment 132440 [details] [review] [14] test-variant: Add a regression test for DBusVariant --- Clean up variant in error path. Assert that we didn't leak memory.
Created attachment 132441 [details] [review] [15] internals: Decouple logging an error from exiting unsuccessfully This lets _dbus_warn() and _dbus_warn_check_failed() fall through to flushing stderr and calling _dbus_abort(), meaning that failed checks and warnings can result in a core dump as intended. By renaming the FATAL severity to ERROR, we ensure that any code contributions that assumed the old semantics will fail to compile. --- I think I like this better than keeping FATAL intact.
(In reply to Simon McVittie from comment #38) > Created attachment 132437 [details] [review] > [11] DBusMessageIter: Add a function to abandon possibly-zero-filled > iterators > > See the doc-comment of the new > dbus_message_iter_abandon_container_if_open() function for details. > > --- > > This is basically Bug #89104, but for iterators. I'm hoping this will make the cleanup code paths over in Bug #101354 (which I don't think we can realistically test with _dbus_test_oom_handling()) more obviously robust against OOM.
Created attachment 132442 [details] [review] [05/15] dbus_message_iter_append_basic: Don't leak signature if appending fd fails --- Avoid -W[error=]unused-label when building for Windows
Comment on attachment 132427 [details] [review] [01] _dbus_marshal_validate_test: Merge two sets of signature validity checks Review of attachment 132427 [details] [review]: ----------------------------------------------------------------- A+, a delightful cleanup, would clean up again.
Comment on attachment 132428 [details] [review] [02] _dbus_marshal_validate_test: Uncomment commented-out test coverage Review of attachment 132428 [details] [review]: ----------------------------------------------------------------- Double-plus-good patch. Excellent VCS archæology.
Comment on attachment 132429 [details] [review] [03] dbus_message_append_args_valist: Don't leak memory on inappropriate type Review of attachment 132429 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 132430 [details] [review] [04] _dbus_message_iter_open_signature: Clarify why this is not leaky Review of attachment 132430 [details] [review]: ----------------------------------------------------------------- ++, assertions for the assertion god.
Comment on attachment 132432 [details] [review] [06] dbus_message_iter_open_container: Don't leak signature on failure Review of attachment 132432 [details] [review]: ----------------------------------------------------------------- ++, free-on-error-paths for the free-on-error-paths throne.
Comment on attachment 132433 [details] [review] [07] internals: Make a minimal _dbus_test_oom_handling() universally available Review of attachment 132433 [details] [review]: ----------------------------------------------------------------- I generally prefer `static inline` functions in the header over function-like #defines, but this is fine. ++
Comment on attachment 132434 [details] [review] [08] test/message: Add a targeted test for recently-fixed leaks Review of attachment 132434 [details] [review]: ----------------------------------------------------------------- The wrong patch seems to have been uploaded here. :-(
Comment on attachment 132435 [details] [review] [09] DBusMessageIter: Clarify the API Review of attachment 132435 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 132436 [details] [review] [10] DBusMessageIter: Zero out the iterator on failure Review of attachment 132436 [details] [review]: ----------------------------------------------------------------- ++ ::: dbus/dbus-message.c @@ +717,5 @@ > +_dbus_message_real_iter_zero (DBusMessageRealIter *iter) > +{ > + _dbus_assert (iter != NULL); > + _DBUS_ZERO (*iter); > + /* NULL is not, strictly speaking, guaranteed to be all-bits-zero */ What?!
Created attachment 132447 [details] [review] [08] test/message: Add a targeted test for recently-fixed leaks --- The right patch this time.
Comment on attachment 132437 [details] [review] [11] DBusMessageIter: Add a function to abandon possibly-zero-filled iterators Review of attachment 132437 [details] [review]: ----------------------------------------------------------------- A couple of comments/questions. ::: dbus/dbus-message.c @@ +722,5 @@ > iter->message = NULL; > } > > +/** > + * Initialize iter as if with DBUS_MESSAGE_ITER_INIT_CLOSED. The only valid s/DBUS_MESSAGE_ITER_INIT_CLOSED/#DBUS_MESSAGE_ITER_INIT_CLOSED/? @@ +3159,5 @@ > + _dbus_return_if_fail (_dbus_message_iter_append_check (real_sub)); > + _dbus_return_if_fail (real_sub->iter_type == DBUS_MESSAGE_ITER_TYPE_WRITER); > +#endif > + > + _dbus_message_iter_abandon_signature (real); Should the abandon_signature() call move up to above the real_iter_is_zeroed(real_sub) check, just in case the real_sub is zeroed, but its container is not? ::: dbus/dbus-message.h @@ +95,5 @@ > + 0, /* dummy11 */ \ > + 0, /* pad1 */ \ > + NULL, /* pad2 */ \ > + NULL /* pad3 */ \ > +} Is it worthwhile adding something like G_STATIC_ASSERT (sizeof (DBUS_MESSAGE_ITER_INIT_CLOSED) == sizeof (DBusMessageIter)); or are we happy that the compiler will warn if assigning `DBusMessageIter iter = DBUS_MESSAGE_ITER_INIT_CLOSED` and one of them has been modified without the other? (This is all pretty marginal, since DBusMessageIter should never change size ever again.)
Comment on attachment 132438 [details] [review] [12] Test dbus_message_iter_abandon_container_if_open under OOM conditions Review of attachment 132438 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 132439 [details] [review] [13] message: Add DBusVariant, a way to copy a single message item Review of attachment 132439 [details] [review]: ----------------------------------------------------------------- I haven’t re-reviewed this in detail, but the handling of contained_signature looks correct.
Comment on attachment 132440 [details] [review] [14] test-variant: Add a regression test for DBusVariant Review of attachment 132440 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 132441 [details] [review] [15] internals: Decouple logging an error from exiting unsuccessfully Review of attachment 132441 [details] [review]: ----------------------------------------------------------------- Yes, this is much nicer.
Comment on attachment 132442 [details] [review] [05/15] dbus_message_iter_append_basic: Don't leak signature if appending fd fails Review of attachment 132442 [details] [review]: ----------------------------------------------------------------- ++
(In reply to Philip Withnall from comment #53) > Comment on attachment 132436 [details] [review] > [10] DBusMessageIter: Zero out the iterator on failure > > > + /* NULL is not, strictly speaking, guaranteed to be all-bits-zero */ > > What?! Seriously. ISO C says converting an integer *constant* with value 0 (possibly with a (void *) cast) yields a null pointer, and all null pointers compare equal; but it leaves it implementation-defined what happens when you convert to and from other integers, including integer variables that happen to contain 0. [§6.3.2.3 in ISO/IEC 9899:201x committee draft N1570, n1570.pdf, which is believed to be functionally identical to the actual ISO C11 standard.] ISO C also says zero-filling a struct containing floating-point or pointer members with calloc() doesn't necessarily mean it contains 0.0 or NULL. [§7.22.3.2.] What we're doing here is the memset() equivalent of what calloc() does to the buffer it is about to return. My understanding is that GLib explicitly doesn't support platforms where NULL isn't all-bits-zero, because they're pretty obscure. Perhaps D-Bus should do the same. http://c-faq.com/null/machexamp.html
Comment on attachment 132447 [details] [review] [08] test/message: Add a targeted test for recently-fixed leaks Review of attachment 132447 [details] [review]: ----------------------------------------------------------------- ++
++ on all patches except for the comments/questions in comment #55.
(In reply to Philip Withnall from comment #55) > s/DBUS_MESSAGE_ITER_INIT_CLOSED/#DBUS_MESSAGE_ITER_INIT_CLOSED/? Yes. > Should the abandon_signature() call move up to above the > real_iter_is_zeroed(real_sub) check, just in case the real_sub is zeroed, > but its container is not? I think I did this deliberately. I'll check again. > Is it worthwhile adding something like > > G_STATIC_ASSERT (sizeof (DBUS_MESSAGE_ITER_INIT_CLOSED) == sizeof > (DBusMessageIter)); > > or are we happy that the compiler will warn if assigning `DBusMessageIter > iter = DBUS_MESSAGE_ITER_INIT_CLOSED` and one of them has been modified > without the other? You can't take sizeof({NULL, 0, 0, blah blah }) because the compiler can't know how large the zeroes are going to be until it knows what they're initializing - they could correspond to anything from a one-bit bitfield to an intmax_t. But it doesn't really matter, because if we supply an incomplete initializer, a C compiler is required to use the same initialization as for static variables for the rest (i.e. pointers are NULL, numbers are zero). If we supply an initializer that's too long, I can't imagine that any reasonable compiler would do anything other than discarding the excess elements (possibly with a warning) or failing compilation. > (This is all pretty marginal, since DBusMessageIter should never change size > ever again.) Indeed. Or if it does change size, we break ABI, and give it a better design and some more padding this time...
Comment on attachment 132437 [details] [review] [11] DBusMessageIter: Add a function to abandon possibly-zero-filled iterators Review of attachment 132437 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-message.c @@ +3159,5 @@ > + _dbus_return_if_fail (_dbus_message_iter_append_check (real_sub)); > + _dbus_return_if_fail (real_sub->iter_type == DBUS_MESSAGE_ITER_TYPE_WRITER); > +#endif > + > + _dbus_message_iter_abandon_signature (real); If real_sub is zeroed, then we know we didn't open it, so we must not close it. I'm being extra-careful here because abandon_signature uses a refcount, so we absolutely need to pair refs, and assertions will not (consistently) save us. The usual situation from my comments: middle = INIT_CLOSED; inner = INIT_CLOSED; if (!open_container (&outer, ..., &middle)) // 1 goto out; if (!open_container (&middle, ..., &inner)) // 2 goto out; if (!close_container (&middle, &inner)) // 3 goto out; if (!close_container (&outer, &middle)) // 4 goto out; result = TRUE; // 5 out: abandon_if_open (&inner, &middle); abandon_if_open (&middle, &outer); ... Imagine we run out of memory and goto out from comment 1. outer is populated but middle is zero. It would be wrong to call abandon_signature(outer), because we never successfully "took a reference" to the signature on behalf of middle. Similarly, if we goto out from comment 2, middle is populated but inner is zero. It would be wrong to call abandon_signature(middle) because inner does not "have a reference". From comment 3, middle and inner are both non-zero. We have to abandon_signature(middle) on behalf of inner. From comment 4 we're back in the same situation as comment 2, and when we fall through at comment 5 we're back in the same situation as at comment 1.
Created attachment 132454 [details] [review] [11] DBusMessageIter: Add a function to abandon possibly-zero-filled iterators See the doc-comment of the new dbus_message_iter_abandon_container_if_open() function for details. Reviewed-by: Philip Withnall <withnall@endlessm.com> [smcv: Add more comments in response to Philip's review] Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101568 --- Philip, I'm assuming that when you put "review+" on this bug's whiteboard, you meant "this is basically fine despite my questions about [11]". I've added more comments to address your concerns and clarify what is going on. If that is not what you meant, please correct me before Travis-CI finishes testing this rebased version :-) I've applied [01] up to [10], plus [15] which was really an orthogonal change, to master for 1.11.14. I also applied the actual bug-fixes ([03], [05], [06]) to dbus-1.10 for 1.10.22.
(In reply to Simon McVittie from comment #66) > Created attachment 132454 [details] [review] [review] > [11] DBusMessageIter: Add a function to abandon possibly-zero-filled > iterators > > See the doc-comment of the new > dbus_message_iter_abandon_container_if_open() function for details. > > Reviewed-by: Philip Withnall <withnall@endlessm.com> > [smcv: Add more comments in response to Philip's review] > Signed-off-by: Simon McVittie <smcv@collabora.com> > Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101568 > > --- > > Philip, I'm assuming that when you put "review+" on this bug's whiteboard, > you meant "this is basically fine despite my questions about [11]". I've > added more comments to address your concerns and clarify what is going on. Correct. The latest version of patch 11 looks good to me too. Thanks for the comments.
Applied in git for 1.11.16 (not 1.11.14 as I said earlier). Memory leak fixes also backported to 1.10.22. Their test passes if I also apply Attachment #132415 [details], but that seemed too intrusive, so I haven't added the new test to 1.10.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.