in _dbus_send_nonce() which has code like below. read_result = _dbus_read_nonce (noncefile, &nonce, error); if (!read_result) { _DBUS_ASSERT_ERROR_IS_SET (error); _dbus_string_free (&nonce); return FALSE; } However, _dbus_read_nonce() may return FALSE if the fopen() noncefile failed. So this will cause _DBUS_ASSERT_ERROR_IS_SET (error); fail and the coredump.
Created attachment 90209 [details] [review] [PATCH] Ensure DBusError is set if _dbus_read_nonce() fail In _dbus_send_nonce() which call in _dbus_read_nonce() and assert on an error is set if _dbus_read_nonce() fail. However, in _dbus_read_nonce(), it may fail on fopen() and left error is unset. This will crash us if assertions hasn't been disabled.
Comment on attachment 90209 [details] [review] [PATCH] Ensure DBusError is set if _dbus_read_nonce() fail Review of attachment 90209 [details] [review]: ----------------------------------------------------------------- Well spotted, but I'd prefer to set a better error. ::: dbus/dbus-nonce.c @@ +112,4 @@ > > > fp = fopen (_dbus_string_get_const_data (fname), "rb"); > + if (!fp || !(nread = fread (buffer, 1, sizeof buffer - 1, fp))) I feel as though the !fp case ought to use errno and _dbus_strerror; perhaps even the function whose name I can't remember that turns a system errno into a DBusError name. Style: I usually prefer to treat assignments as statements rather than using them as rvalues (with occasional exceptions for "for" and "while" loops) - I think temp = thing (); if (!temp) { error (); return; } do_something (temp); is clearer than if (!(temp = thing ())) { error (); return; } do_something (temp); @@ +116,2 @@ > { > dbus_set_error (error, DBUS_ERROR_FILE_NOT_FOUND, "Could not read nonce from file %s", _dbus_string_get_const_data (fname)); Strictly speaking, FILE_NOT_FOUND is not the right error for the existing "!nread" case anyway: if we successfully opened the file but could not read any bytes from it, that's not "file not found".
(In reply to comment #2) > Comment on attachment 90209 [details] [review] [review] > [PATCH] Ensure DBusError is set if _dbus_read_nonce() fail > > Review of attachment 90209 [details] [review] [review]: > ----------------------------------------------------------------- > > Well spotted, but I'd prefer to set a better error. > > ::: dbus/dbus-nonce.c > @@ +112,4 @@ > > > > > > fp = fopen (_dbus_string_get_const_data (fname), "rb"); > > + if (!fp || !(nread = fread (buffer, 1, sizeof buffer - 1, fp))) > > I feel as though the !fp case ought to use errno and _dbus_strerror; perhaps > even the function whose name I can't remember that turns a system errno into > a DBusError name. Yes, it's _dbus_error_from_errno() > > Style: I usually prefer to treat assignments as statements rather than using > them as rvalues (with occasional exceptions for "for" and "while" loops) - I > think > > temp = thing (); > > if (!temp) > { > error (); > return; > } > > do_something (temp); > > is clearer than > > if (!(temp = thing ())) > { > error (); > return; > } > > do_something (temp); > sure, definitely agree. > @@ +116,2 @@ > > { > > dbus_set_error (error, DBUS_ERROR_FILE_NOT_FOUND, "Could not read nonce from file %s", _dbus_string_get_const_data (fname)); > > Strictly speaking, FILE_NOT_FOUND is not the right error for the existing > "!nread" case anyway: if we successfully opened the file but could not read > any bytes from it, that's not "file not found". Yes, I original though I may add a new ERROR type, but then I though "Could not read nonce from file %s" is well enough to cover the file can not open. :-).
Created attachment 90329 [details] [review] [PATCH v2] Ensure DBusError is set if _dbus_read_nonce() fail In _dbus_send_nonce() which call in _dbus_read_nonce() and assert on an error is set if _dbus_read_nonce() fail. However, in _dbus_read_nonce(), it may fail on fopen() and left error is unset. This will crash us if assertions hasn't been disabled. This patch doesn't add any new DBUS_ERROR_, as I saw that the error may happen mostly ENOENT, EACCESS are already here.
Comment on attachment 90329 [details] [review] [PATCH v2] Ensure DBusError is set if _dbus_read_nonce() fail Review of attachment 90329 [details] [review]: ----------------------------------------------------------------- Looks good, thanks
Fixed in git for 1.7.10, thanks
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.