Bug 72298

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