Notes and questions about remaining todo's and FIXME's marked 1.0

[Havoc Pennington]

John (J5) Palmieri wrote:
> mclausen and I looked over this.  Since you can only unlink sockets you
> have permission to there is no huge implication here.  You could unlink
> another D-Bus socket but in practice this would be very unlikely.
> Proposal is to remove the 1st FIXME and change the second FIXME to just
> a comment.

Sounds good.

> 
> ***********************************************************************
> _dbus_change_identity
> /* setgroups() only works if we are a privileged process,
>    * so we don't return error on failure; the only possible
>    * failure is that we don't have perms to do it.
>    * FIXME 1.0 not sure this is right, maybe if setuid()
>    * is going to work then setgroups() should also work.
>    */
>   if (setgroups (0, NULL) < 0)
>     _dbus_warn ("Failed to drop supplementary groups: %s\n",
>                 _dbus_strerror (errno));
> 
>   /* Set GID first, or the setuid may remove our permission
>    * to change the GID
>    */
>   if (setgid (gid) < 0)
>     {
>       dbus_set_error (error, _dbus_error_from_errno (errno),
>                       "Failed to set GID to %lu: %s", gid,
>                       _dbus_strerror (errno));
>       return FALSE;
>     }
> 
> Should we change this to return an error or not?

I have no idea thus the FIXME ;-) but probably just changing this to a 
comment is fine, since if setuid is going to fail that will return an error.

> **********************************************************************
> bus_connection_disconnected
> /* Drop any service ownership. FIXME 1.0? Unfortunately, this requires
>    * memory allocation and there doesn't seem to be a good way to
>    * handle it other than sleeping; we can't "fail" the operation of
>    * disconnecting a client, and preallocating a broadcast "service is
>    * now gone" message for every client-service pair seems kind of
>    * involved. Probably we need to do that though.
>    */
>    
> Is this important for 1.0?  How would I go about preallocating "service
> is now gone" message for every client-service pair broadcast?

This is effectively unfixable I believe. You could just change it to be 
a comment. (I'm assuming it does in fact do the sleeping.)

> ***********************************************************************
> try_send_activation_failure
> /**
>  * FIXME @todo 1.0? the error messages here would ideally be
> preallocated
>  * so we don't need to allocate memory to send them.
>  * Using the usual tactic, prealloc an OOM message, then
>  * if we can't alloc the real error send the OOM error instead.
>  */
> 
> This looks involved because even if you had a preallocated message
> you would need to set the destination which could throw an OOM error.
> Is it important for 1.0?

I don't think it's 1.0 important.

> **********************************************************************
> DBusKeyring
> * @todo 1.0? there's a memory leak on some codepath in here, I saw it
> once
>  * when running make check - probably some specific initial cookies
>  * present in the cookie file, then depending on what we do with them.
> 
> Any clue if this is still an issue.  Can it be tracked down? 

No clue. The only way I know to approach this would be to just increase 
the test coverage on DBusKeyring and try to handle some more cases such 
as cookie files with several things in them, cookie files with expired 
cookies, etc.

> **********************************************************************
> _dbus_setenv
>  * @todo 1.0 if someone can verify it's safe, we could avoid the
>  * memleak when doing an unset.
> 
> Nalin seems to think there is no safe way of doing this

Yeah, I don't think there is either. Could just leave a "memleak here, 
not fixable" comment.

> **********************************************************************
> _dbus_connection_queue_synthesized_message_link
> * @todo 1.0? This needs to wake up the mainloop if it is in
> * a poll/select and this is a multithreaded app.
> 
> Not sure what this todo is getting at.  We already do
> _dbus_connection_wakeup_mainloop (connection); in this function.

Maybe in the CVS logs the @todo predates that wakeup_mainloop call? Or 
maybe wakeup_mainloop isn't always called?

> _dbus_auth_decode_data
> * @todo 1.0? We need to be able to distinguish "out of memory" error
> * from "the data is hosed" error.
> 
> The problem here is we call into virtual methods:
> if (DBUS_AUTH_IS_CLIENT (auth))
>         return (* auth->mech->client_decode_func) (auth, encoded,
> plaintext);
>       else
>         return (* auth->mech->server_decode_func) (auth, encoded,
> plaintext);
>         
> Can those methods return OOM?  If so the solution would be to pass a
> DBusError to them.  Where are the implementations for client_decode_func
> and server_decode_func?

I think they're all in dbus-auth.c somewhere aren't they?

> *************************************************************************
> dbus_pending_call_block
>  * @todo 1.0? when you start blocking, the timeout is reset, but it
> should
>  * really only use time remaining since the pending call was created.
> 
> This is a bit more involved than I first thought.  Timeouts are done in
> intervals and not based on time stamps.  Not a big issue so I think we
> should punt this for now.  It can be fixed later.
> 

Yeah, I don't think it's a blocker. Should remain marked for @todo though.

Havoc