Bug 101568 - Add a way to store single items from messages
Summary: Add a way to store single items from messages
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium enhancement
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL: https://github.com/smcv/dbus/commits/...
Whiteboard: review+
Keywords: patch
Depends on:
Blocks: 101354
  Show dependency treegraph
 
Reported: 2017-06-23 16:12 UTC by Simon McVittie
Modified: 2017-07-05 16:52 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
message: Add DBusVariant, a way to copy a single message item (10.35 KB, patch)
2017-06-23 16:13 UTC, Simon McVittie
Details | Splinter Review
test-variant: Add a regression test for DBusVariant (15.97 KB, patch)
2017-06-23 16:13 UTC, Simon McVittie
Details | Splinter Review
message: Add DBusVariant, a way to copy a single message item (10.50 KB, patch)
2017-07-03 18:58 UTC, Simon McVittie
Details | Splinter Review
internals: Add a log severity that looks like FATAL, but does not _exit() (2.73 KB, patch)
2017-07-03 19:03 UTC, Simon McVittie
Details | Splinter Review
_dbus_message_set_signature: Delete unused function (2.83 KB, patch)
2017-07-03 19:59 UTC, Simon McVittie
Details | Splinter Review
DBusMessage: Stop using _dbus_check_is_valid_signature() (3.75 KB, patch)
2017-07-03 19:59 UTC, Simon McVittie
Details | Splinter Review
Remove now-unused _dbus_check_is_valid_signature() (1.43 KB, patch)
2017-07-03 19:59 UTC, Simon McVittie
Details | Splinter Review
Remove now-unused _dbus_validate_signature() (5.09 KB, patch)
2017-07-03 20:00 UTC, Simon McVittie
Details | Splinter Review
test-variant: Add a regression test for DBusVariant (18.22 KB, patch)
2017-07-03 20:01 UTC, Simon McVittie
Details | Splinter Review
[01] _dbus_marshal_validate_test: Merge two sets of signature validity checks (3.75 KB, patch)
2017-07-04 17:54 UTC, Simon McVittie
Details | Splinter Review
[02] _dbus_marshal_validate_test: Uncomment commented-out test coverage (1.47 KB, patch)
2017-07-04 17:55 UTC, Simon McVittie
Details | Splinter Review
[03] dbus_message_append_args_valist: Don't leak memory on inappropriate type (968 bytes, patch)
2017-07-04 17:56 UTC, Simon McVittie
Details | Splinter Review
[04] _dbus_message_iter_open_signature: Clarify why this is not leaky (1.05 KB, patch)
2017-07-04 17:56 UTC, Simon McVittie
Details | Splinter Review
[05] dbus_message_iter_append_basic: Don't leak signature if appending fd fails (1.48 KB, patch)
2017-07-04 17:57 UTC, Simon McVittie
Details | Splinter Review
[06] dbus_message_iter_open_container: Don't leak signature on failure (3.22 KB, patch)
2017-07-04 17:58 UTC, Simon McVittie
Details | Splinter Review
[07] internals: Make a minimal _dbus_test_oom_handling() universally available (2.42 KB, patch)
2017-07-04 17:58 UTC, Simon McVittie
Details | Splinter Review
[08] test/message: Add a targeted test for recently-fixed leaks (1.05 KB, patch)
2017-07-04 18:02 UTC, Simon McVittie
Details | Splinter Review
[09] DBusMessageIter: Clarify the API (3.08 KB, patch)
2017-07-04 18:02 UTC, Simon McVittie
Details | Splinter Review
[10] DBusMessageIter: Zero out the iterator on failure (3.70 KB, patch)
2017-07-04 18:03 UTC, Simon McVittie
Details | Splinter Review
[11] DBusMessageIter: Add a function to abandon possibly-zero-filled iterators (6.85 KB, patch)
2017-07-04 18:04 UTC, Simon McVittie
Details | Splinter Review
[12] Test dbus_message_iter_abandon_container_if_open under OOM conditions (3.18 KB, patch)
2017-07-04 18:04 UTC, Simon McVittie
Details | Splinter Review
[13] message: Add DBusVariant, a way to copy a single message item (10.41 KB, patch)
2017-07-04 18:05 UTC, Simon McVittie
Details | Splinter Review
[14] test-variant: Add a regression test for DBusVariant (18.44 KB, patch)
2017-07-04 18:05 UTC, Simon McVittie
Details | Splinter Review
[15] internals: Decouple logging an error from exiting unsuccessfully (6.71 KB, patch)
2017-07-04 18:07 UTC, Simon McVittie
Details | Splinter Review
[05/15] dbus_message_iter_append_basic: Don't leak signature if appending fd fails (1.84 KB, patch)
2017-07-04 18:31 UTC, Simon McVittie
Details | Splinter Review
[08] test/message: Add a targeted test for recently-fixed leaks (8.00 KB, patch)
2017-07-05 11:24 UTC, Simon McVittie
Details | Splinter Review
[11] DBusMessageIter: Add a function to abandon possibly-zero-filled iterators (8.43 KB, patch)
2017-07-05 14:59 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2017-06-23 16:12:51 UTC
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.)
Comment 1 Simon McVittie 2017-06-23 16:13:15 UTC
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.
Comment 2 Simon McVittie 2017-06-23 16:13:29 UTC
Created attachment 132162 [details] [review]
test-variant: Add a regression test for DBusVariant
Comment 3 Philip Withnall 2017-07-03 11:10:49 UTC
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 */
  }

?
Comment 4 Simon McVittie 2017-07-03 11:14:19 UTC
(In reply to Philip Withnall from comment #3)
> Some unit tests would be nice

They are the other commit here.
Comment 5 Philip Withnall 2017-07-03 11:21:13 UTC
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 6 Simon McVittie 2017-07-03 13:22:07 UTC
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.
Comment 7 Simon McVittie 2017-07-03 18:58:12 UTC
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.
Comment 8 Simon McVittie 2017-07-03 19:03:10 UTC
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.
Comment 9 Simon McVittie 2017-07-03 19:59:06 UTC
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.
Comment 10 Simon McVittie 2017-07-03 19:59:38 UTC
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.
Comment 11 Simon McVittie 2017-07-03 19:59:57 UTC
Created attachment 132416 [details] [review]
Remove now-unused _dbus_check_is_valid_signature()

As noted in the previous commit, it's a trap.
Comment 12 Simon McVittie 2017-07-03 20:00:27 UTC
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.
Comment 13 Simon McVittie 2017-07-03 20:01:20 UTC
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.
Comment 14 Simon McVittie 2017-07-03 20:04:44 UTC
There is a memory leak somewhere which I still need to track down.
Comment 15 Philip Withnall 2017-07-04 09:24:26 UTC
Comment on attachment 132412 [details] [review]
message: Add DBusVariant, a way to copy a single message  item

Review of attachment 132412 [details] [review]:
-----------------------------------------------------------------

++
Comment 16 Philip Withnall 2017-07-04 09:28:51 UTC
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 17 Philip Withnall 2017-07-04 09:29:11 UTC
Comment on attachment 132414 [details] [review]
_dbus_message_set_signature: Delete unused function

Review of attachment 132414 [details] [review]:
-----------------------------------------------------------------

Trivially yes. ++
Comment 18 Philip Withnall 2017-07-04 09:36:37 UTC
Comment on attachment 132415 [details] [review]
DBusMessage: Stop using _dbus_check_is_valid_signature()

Review of attachment 132415 [details] [review]:
-----------------------------------------------------------------

++
Comment 19 Philip Withnall 2017-07-04 09:37:21 UTC
Comment on attachment 132416 [details] [review]
Remove now-unused _dbus_check_is_valid_signature()

Review of attachment 132416 [details] [review]:
-----------------------------------------------------------------

++
Comment 20 Philip Withnall 2017-07-04 09:40:43 UTC
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 21 Philip Withnall 2017-07-04 09:43:28 UTC
Comment on attachment 132418 [details] [review]
test-variant: Add a regression test for DBusVariant

Review of attachment 132418 [details] [review]:
-----------------------------------------------------------------

++
Comment 22 Simon McVittie 2017-07-04 15:48:18 UTC
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.
Comment 23 Simon McVittie 2017-07-04 16:06:38 UTC
(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 24 Simon McVittie 2017-07-04 17:50:25 UTC
Comment on attachment 132414 [details] [review]
_dbus_message_set_signature: Delete unused function

Applied
Comment 25 Simon McVittie 2017-07-04 17:50:37 UTC
Comment on attachment 132415 [details] [review]
DBusMessage: Stop using _dbus_check_is_valid_signature()

Applied
Comment 26 Simon McVittie 2017-07-04 17:50:54 UTC
Comment on attachment 132416 [details] [review]
Remove now-unused _dbus_check_is_valid_signature()

Applied
Comment 27 Simon McVittie 2017-07-04 17:52:09 UTC
Comment on attachment 132417 [details] [review]
Remove now-unused _dbus_validate_signature()

Applied, thanks for the reviews so far!
Comment 28 Simon McVittie 2017-07-04 17:54:47 UTC
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.
Comment 29 Simon McVittie 2017-07-04 17:55:36 UTC
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.
Comment 30 Simon McVittie 2017-07-04 17:56:24 UTC
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.
Comment 31 Simon McVittie 2017-07-04 17:56:52 UTC
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.
Comment 32 Simon McVittie 2017-07-04 17:57:24 UTC
Created attachment 132431 [details] [review]
[05] dbus_message_iter_append_basic: Don't leak signature if  appending fd fails

---

Test to follow.
Comment 33 Simon McVittie 2017-07-04 17:58:00 UTC
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.
Comment 34 Simon McVittie 2017-07-04 17:58:57 UTC
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.
Comment 35 Simon McVittie 2017-07-04 18:02:01 UTC
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.
Comment 36 Simon McVittie 2017-07-04 18:02:30 UTC
Created attachment 132435 [details] [review]
[09] DBusMessageIter: Clarify the API

Having opened a container for appending, the container must be closed
exactly once.
Comment 37 Simon McVittie 2017-07-04 18:03:29 UTC
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.
Comment 38 Simon McVittie 2017-07-04 18:04:14 UTC
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.
Comment 39 Simon McVittie 2017-07-04 18:04:29 UTC
Created attachment 132438 [details] [review]
[12] Test dbus_message_iter_abandon_container_if_open under  OOM conditions
Comment 40 Simon McVittie 2017-07-04 18:05:23 UTC
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.
Comment 41 Simon McVittie 2017-07-04 18:05:54 UTC
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.
Comment 42 Simon McVittie 2017-07-04 18:07:21 UTC
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.
Comment 43 Simon McVittie 2017-07-04 18:11:46 UTC
(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.
Comment 44 Simon McVittie 2017-07-04 18:31:54 UTC
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 45 Philip Withnall 2017-07-05 11:01:17 UTC
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 46 Philip Withnall 2017-07-05 11:05:25 UTC
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 47 Philip Withnall 2017-07-05 11:06:35 UTC
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 48 Philip Withnall 2017-07-05 11:09:22 UTC
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 49 Philip Withnall 2017-07-05 11:11:17 UTC
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 50 Philip Withnall 2017-07-05 11:13:14 UTC
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 51 Philip Withnall 2017-07-05 11:15:18 UTC
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 52 Philip Withnall 2017-07-05 11:16:25 UTC
Comment on attachment 132435 [details] [review]
[09] DBusMessageIter: Clarify the API

Review of attachment 132435 [details] [review]:
-----------------------------------------------------------------

++
Comment 53 Philip Withnall 2017-07-05 11:20:30 UTC
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?!
Comment 54 Simon McVittie 2017-07-05 11:24:58 UTC
Created attachment 132447 [details] [review]
[08] test/message: Add a targeted test for recently-fixed  leaks

---

The right patch this time.
Comment 55 Philip Withnall 2017-07-05 11:29:44 UTC
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 56 Philip Withnall 2017-07-05 11:32:05 UTC
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 57 Philip Withnall 2017-07-05 11:33:27 UTC
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 58 Philip Withnall 2017-07-05 11:37:33 UTC
Comment on attachment 132440 [details] [review]
[14] test-variant: Add a regression test for DBusVariant

Review of attachment 132440 [details] [review]:
-----------------------------------------------------------------

++
Comment 59 Philip Withnall 2017-07-05 11:39:15 UTC
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 60 Philip Withnall 2017-07-05 11:41:25 UTC
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]:
-----------------------------------------------------------------

++
Comment 61 Simon McVittie 2017-07-05 11:42:22 UTC
(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 62 Philip Withnall 2017-07-05 11:47:21 UTC
Comment on attachment 132447 [details] [review]
[08] test/message: Add a targeted test for recently-fixed  leaks

Review of attachment 132447 [details] [review]:
-----------------------------------------------------------------

++
Comment 63 Philip Withnall 2017-07-05 11:48:49 UTC
++ on all patches except for the comments/questions in comment #55.
Comment 64 Simon McVittie 2017-07-05 11:51:08 UTC
(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 65 Simon McVittie 2017-07-05 12:10:23 UTC
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.
Comment 66 Simon McVittie 2017-07-05 14:59:28 UTC
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.
Comment 67 Philip Withnall 2017-07-05 15:11:14 UTC
(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.
Comment 68 Simon McVittie 2017-07-05 16:52:39 UTC
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.