Created attachment 135264 [details] [review] _DBUS_STRING_DEFINE_STATIC: Don't include zero termination in length _dbus_string_init_const() doesn't include the trailing '\0' in the DBusRealString->len, so this surely shouldn't either. In practice none of the users of _DBUS_STRING_DEFINE_STATIC care one way or the other, but it seems better to be consistent. --- This does not provide the requested API, it's only preparation. Created attachment 135265 [details] [review] tests: Assert that _DBUS_STRING_DEFINE_STATIC looks as expected --- This fails prior to Attachment #135264 [details]. Comment on attachment 135264 [details] [review] _DBUS_STRING_DEFINE_STATIC: Don't include zero termination in length Review of attachment 135264 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 135265 [details] [review] tests: Assert that _DBUS_STRING_DEFINE_STATIC looks as expected Review of attachment 135265 [details] [review]: ----------------------------------------------------------------- yup, r+ (In reply to Simon McVittie from comment #0) > It should be possible to do this with DBusString, like you can already do > the equivalent with DBusError: > > DBusString one = DBUS_STRING_INIT; > DBusString two = DBUS_STRING_INIT; > dbus_bool_t ret = FALSE; > > if (!_dbus_string_init (&one)) > goto out; > > /* do some more things with &one that might "goto out" */ > > if (!_dbus_string_init (&two)) > goto out; > > /* do some more things with &one and/or &two that might "goto out" */ > > ret = TRUE; > > out: > _dbus_string_free (&one); > _dbus_string_free (&two); > return ret; Another possibility that I've considered in the past was to introduce a DBUS_STRING_INIT_INVALID or possibly DBUS_STRING_INIT_NULL, which behaves like NULL: it's a valid initializer, and either the free-function can detect and ignore it (like strdup/free) or the caller of the free-function can ignore it explicitly (like dbus_message_new/dbus_message_unref), but it isn't valid for any other use (like the way you can't strcmp NULL, and you can't validly call methods on a NULL DBusMessage *). Comment on attachment 135264 [details] [review] _DBUS_STRING_DEFINE_STATIC: Don't include zero termination in length Thanks, applied for 1.13.0. Comment on attachment 135265 [details] [review] tests: Assert that _DBUS_STRING_DEFINE_STATIC looks as expected Thanks, applied for 1.13.0. The code touched by Bug #103597 is a prime candidate for using this. So is Bug #101354. Created attachment 135634 [details] [review] [01/12] DBusString: Reverse the sense of ->invalid It's easier to implement a stack-allocated string that is valid to free (but for no other purpose) if we consider all-bits-zero to be invalid. Created attachment 135635 [details] [review] 02/12] DBusString: Add _DBUS_STRING_INIT_INVALID and allow "freeing" it This means we can finally use patterns like this: DBusString buffer = _DBUS_STRING_INIT_INVALID; dbus_bool_t ret = FALSE; ... some long setup ... if (!_dbus_string_init (&buffer)) goto out; ... some long operation ... ret = TRUE; out: ... free things ... _dbus_string_free (&buffer); ... free more things ... return ret; without having to have a separate boolean to track whether buffer has been initialized. One observable difference is that if s is a "const" (borrowed pointer) string, _dbus_string_free (&s) now sets it to be invalid. Previously, it would have kept its (borrowed pointer) contents, which seems like a violation of least-astonishment. Created attachment 135636 [details] [review] [03/12] _dbus_transport_new_for_socket: Simplify with _DBUS_STRING_INIT_INVALID This is one of the few places that has test coverage for all the OOM code paths. It was also one of the worst (most complicated) error-unwinding locations, with labels failed_0 up to failed_4. Created attachment 135638 [details] [review] [04/12] _dbus_server_new_for_socket: Invalidate watches during error unwinding We assert that every watch is invalidated before it is freed, but in some OOM code paths this didn't happen. --- Suggestions welcome on whether this should be cherry-picked to 1.12.x. I wanted to convert several more of the worst offenders to use _DBUS_STRING_INIT_INVALID, but it turned out they didn't have OOM test coverage... and when I added test coverage, I had to fix pre-existing bugs in the OOM code paths before it would pass. “There’s also a historical note; I wrote a lot of [dbus] 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 – the handling code crashed, locked up, or something.” — Havoc Pennington, in https://blog.ometer.com/2008/02/04/out-of-memory-handling-d-bus-experience/ 9 years after that, and 14 years after the addition of _dbus_test_oom_handling(), we are *still* finding broken OOM code paths when we test them. I think we can conclude that in the real world, malloc() failing is a vanishingly rare situation. Created attachment 135639 [details] [review] [05/12] _dbus_server_new_for_socket: Properly disconnect during error unwinding _dbus_server_finalize_base() asserts that the socket has been disconnected, but in some OOM code paths we would call it without officially disconnecting. Do so. This means we need to be a bit more careful about what is socket_disconnect()'s responsibility to clean up, what is _dbus_server_new_for_socket()'s responsibility, and what is the caller's responsibility. --- Possibly a candidate for 1.12.x? But nobody has noticed this in the last decade so maybe we don't actually care? Created attachment 135640 [details] [review] [06/12] [UNTESTED] _dbus_server_new_for_launchd: Don't leak fd on failure If _dbus_server_new_for_socket() fails, it is the caller's responsibility to close the fds. All other callers did this. --- I don't use Mac OS, or OS X, or macOS, or whatever it's officially called now, so I can't test this. Perhaps I should just apply it anyway. Created attachment 135641 [details] [review] [07/12] _dbus_server_new_for_tcp_socket: Don't pile up errors on OOM If _dbus_noncefile_create() has failed and set error, it is incorrect for us to set it again. Created attachment 135642 [details] [review] [08/12] _dbus_listen_tcp_socket: Don't rely on dbus_realloc setting errno dbus_realloc() doesn't guarantee to set errno (if it did, the only reasonable thing it could set it to would be ENOMEM). In particular, faking OOM conditions doesn't set it. This can cause an assertion failure when OOM tests assert that the only error that can validly occur is DBUS_ERROR_NO_MEMORY. Created attachment 135643 [details] [review] [09/12] dbus-nonce: Don't crash on encountering OOM Created attachment 135644 [details] [review] [10/12] Add a targeted test for OOM during _dbus_server_new_for_tcp_socket() This also covers _dbus_server_new_for_socket(), which is one of the worse places in terms of complexity of the error-unwinding path (3 labels). Created attachment 135645 [details] [review] [11/12] _dbus_server_new_for_socket: Simplify error unwinding --- This doesn't actually use _DBUS_STRING_INIT_INVALID at all. Created attachment 135646 [details] [review] [12/12] _dbus_server_new_for_tcp_socket: Simplify error unwinding (In reply to Simon McVittie from comment #16) > [08/12] _dbus_listen_tcp_socket: Don't rely on dbus_realloc setting errno The Windows equivalent of this Unix-specific code was already correct, so doesn't need changing. Comment on attachment 135634 [details] [review] [01/12] DBusString: Reverse the sense of ->invalid Review of attachment 135634 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135635 [details] [review] 02/12] DBusString: Add _DBUS_STRING_INIT_INVALID and allow "freeing" it Review of attachment 135635 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135636 [details] [review] [03/12] _dbus_transport_new_for_socket: Simplify with _DBUS_STRING_INIT_INVALID Review of attachment 135636 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135638 [details] [review] [04/12] _dbus_server_new_for_socket: Invalidate watches during error unwinding Review of attachment 135638 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135639 [details] [review] [05/12] _dbus_server_new_for_socket: Properly disconnect during error unwinding Review of attachment 135639 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135640 [details] [review] [06/12] [UNTESTED] _dbus_server_new_for_launchd: Don't leak fd on failure Review of attachment 135640 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135641 [details] [review] [07/12] _dbus_server_new_for_tcp_socket: Don't pile up errors on OOM Review of attachment 135641 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135642 [details] [review] [08/12] _dbus_listen_tcp_socket: Don't rely on dbus_realloc setting errno Review of attachment 135642 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135643 [details] [review] [09/12] dbus-nonce: Don't crash on encountering OOM Review of attachment 135643 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135644 [details] [review] [10/12] Add a targeted test for OOM during _dbus_server_new_for_tcp_socket() Review of attachment 135644 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135645 [details] [review] [11/12] _dbus_server_new_for_socket: Simplify error unwinding Review of attachment 135645 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135646 [details] [review] [12/12] _dbus_server_new_for_tcp_socket: Simplify error unwinding Review of attachment 135646 [details] [review]: ----------------------------------------------------------------- ++ I pushed these (and forgot to close the bug), but one of the bug-fixing patches was wrong. Comment on attachment 135639 [details] [review] [05/12] _dbus_server_new_for_socket: Properly disconnect during error unwinding Review of attachment 135639 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-server-socket.c @@ +343,5 @@ > + /* The caller is still responsible for closing the fds until > + * we return successfully, so don't let socket_disconnect() > + * close them */ > + for (j = 0; j < n_fds; j++) > + _dbus_socket_invalidate (&socket_server->fds[i]); This needs to invalidate each fd indexed by j in [0, n_fds), not invalidate fd i (which is the one we failed to watch) repeatedly. @@ +352,5 @@ > + for (j = i; j < n_fds; j++) > + { > + _dbus_watch_invalidate (socket_server->watch[i]); > + _dbus_watch_unref (socket_server->watch[i]); > + socket_server->watch[i] = NULL; Similarly, this needs to invalidate and unref each watch indexed by j in [i, n_fds) (that is, exactly the ones that socket_disconnect() can't be allowed to see because they weren't added to the main loop yet), not invalidate watch i repeatedly. Created attachment 135738 [details] [review] [13] _dbus_server_new_for_socket: Iterate over arrays as intended Commit 0c03b505 was meant to clear all the fds indexed by j in [0, n_fds), which socket_disconnect() can't be allowed to close (because on failure the caller remains responsible for closing them); but instead it closed the one we failed to add to the main loop (fd i), repeatedly. Similarly, it was meant to invalidate all the watches indexed by j in [i, n_fds) (the one we failed to add to the main loop and the ones we didn't try to add to the main loop yet), which socket_disconnect() can't be allowed to see (because it would fail to remove them from the main loop and hit an assertion failure); but instead it invalidated fd i, repeatedly. These happen to be the same thing if you only have one fd, resulting in the test-case passing on an IPv4-only system, but failing on a system with both IPv4 and IPv6. --- One of the lesser-known assumptions that is broken by having IPv6! (In reply to Simon McVittie from comment #34) > I pushed these (and forgot to close the bug), but one of the bug-fixing > patches was wrong. Oh sigh. My bad for missing that in the review, sorry. Comment on attachment 135738 [details] [review] [13] _dbus_server_new_for_socket: Iterate over arrays as intended Review of attachment 135738 [details] [review]: ----------------------------------------------------------------- ++ yes yes Thanks. I'll push this when I've run more thorough tests on it. Thanks, fixed in git for 1.13.0, with patches 04, 05, 08, 10, 13 backported for 1.12.4. Patches 07, 09 appear to have been fixes for regressions in master and so are not applicable to 1.12. I can't test patch 06 so I'm not backporting that one either; happy to backport it if someone tests it on macOS. |
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.
It should be possible to do this with DBusString, like you can already do the equivalent with DBusError: DBusString one = DBUS_STRING_INIT; DBusString two = DBUS_STRING_INIT; dbus_bool_t ret = FALSE; if (!_dbus_string_init (&one)) goto out; /* do some more things with &one that might "goto out" */ if (!_dbus_string_init (&two)) goto out; /* do some more things with &one and/or &two that might "goto out" */ ret = TRUE; out: _dbus_string_free (&one); _dbus_string_free (&two); return ret; At the moment, the closest you can get is: DBusString one; DBusString two; dbus_bool_t ret = FALSE; /* this takes advantage of the fact that this cannot fail, * and does not actually allocate memory */ _dbus_string_init_const (&one, ""); _dbus_string_init_const (&two, ""); if (!_dbus_string_init (&one)) goto out; /* do some more things with &one that might "goto out" */ if (!_dbus_string_init (&two)) goto out; /* do some more things with &one and/or &two that might "goto out" */ ret = TRUE; out: _dbus_string_free (&one); _dbus_string_free (&two); return ret;