Re: [Rhythmbox-devel] music sharing patch #3



On Tue, 2005-08-23 at 00:22 -0400, Charles Schmidt wrote:
> -
> Yet another music sharing patch for people to play with.  This should be
> mostly final (in the sense that maybe this could find its way into cvs
> and be worked on from there, soon?).

Yep; do you have a GNOME CVS account already?  If not:
http://developer.gnome.org/doc/policies/accounts/requesting.html

> The DNS-SD/Bonjour/Rendezvous isn't done via gnome-vfs anymore
> (previously the daap patch used gnome-vfs [which used howl] to
> browse/discovery daap hosts).  It is done directly via howl now.  I've
> abstracted that part of the code out so that a drop in replacement
> (avahi) should be relatively easy.

Ok.

> You'll need libsoup & howl to browse music and publish your own.  I'm
> planning on writing in support for avahi
> ( http://www.freedesktop.org/Software/Avahi ) as soon as I get it
> working on my machine (damn dbus).  Hopefully between the two of them
> people will be able to set up mdns themselves (or, with avahi, have it
> come with their distribution).

Yeah, the OS has to provide this feature, I don't see how we can.

> As before, you'll still need the patch at
> http://bugzilla.gnome.org/show_bug.cgi?id=312114 against gst-plugins.

Ok...I'm compiling gstreamer 0.9 now to see if I can get this patch into
HEAD, and from there backported to 0.8.

More comments on the patch:

+	headers = g_string_new ("Accept: */*\r\nCache-Control: no-cache\r\nUser-Agent: iTunes/4.6 (Windows; N)\r\nClient-DAAP-Access-Index: 2\r\nClient-DAAP-Version: 3.0\r\n");

Shouldn't this use RB_DAAP_USER_AGENT?

+static const char appleCopyright[] = "Copyright 2003 Apple Computer, Inc.";

I suggest we hex-encode (or something) this, and de-hex it at runtime.
That avoids any possible misinterpretation of it as an actual copyright
claim, as opposed to a byte array.

> --- rhythmbox/rhythmdb/rhythmdb.c       2005-08-17 05:37:42.000000000
> -0400
> +++ myrhythmbox/rhythmdb/rhythmdb.c     2005-08-22 21:40:26.000000000
> -0400
> @@ -1611,6 +1611,18 @@
> 
> GNOME_VFS_FILE_INFO_FOLLOW_LINKS))
>             == GNOME_VFS_OK)
>                 return;
> +       if (g_str_has_prefix (uri, "daap://")) {
> +               GnomeVFSFileInfo *info = event->vfsinfo;
> +
> +               info->name = g_strdup(strrchr(uri,'/') + 1);
> +               info->valid_fields =
> GNOME_VFS_FILE_INFO_FIELDS_PERMISSIONS |
> GNOME_VFS_FILE_INFO_FIELDS_TYPE | GNOME_VFS_FILE_INFO_FIELDS_FLAGS;
> +
> +               info->type = GNOME_VFS_FILE_TYPE_REGULAR;
> +               info->flags = GNOME_VFS_FILE_FLAGS_NONE;
> +               info->permissions = GNOME_VFS_PERM_USER_READ;
> +               
> +               return;
> +       }
>  error:

Is this still necessary?  I fixed a bug a while back in the core that
statted all entries, not just songs.


>               if (buf_length < codesize + 8) {
> +                       g_warning ("Malformed response");
> +                       return;
> +               }

Two things here.
First, I'd prefer if we didn't g_warning for this.  Potentially it will
happen due to malicious input, better we don't fill up the user's
~/.xsession-log or whatever; attacks should just silently fail.  

Second, the "+ 8" there has a few problems.  Won't it potentially cause
us to lose the last daap item?  Or does the DAAP protocol have a padding
of 8?  Also, this is vulnerable to an integer overflow; if the attacker
specifies G_MAXINT, + 8 will overflow to a negative value, passing your
check.   Also, when the check returns, we leak item and node.

I know it's a pain, but getting this stuff right is very important.  

> Issues/Known Bugs:
> * Signed errors when compiling.  According to walters (i think), gcc 4.0
> added the signed checking thing.  I havn't found time to find packages
> for 4.0 for my system yet, or I'd try to fix this myself.  Until then, I
> suggest writing me a patch or turning off the extra warning checking.

I think James had a patch.

> * The patch from http://bugzilla.gnome.org/show_bug.cgi?id=167364
> "broke" seeking for daap streams.  You can still seek in them just fine,
> but because the GStreamer playbin isn't aware that you have, it can't
> report the time properly.  While I agree that the "new" way
> (gst_element_query) is better, I can't see any way to get the time
> reported for daap streams except to do it the "old" way.  Any thoughts?

Hmmm.  

> * There isn't really any error reporting yet.
> * When sharing your music, if the share name already exists on the
> network, Rhythmbox won't notify you of this.

Hopefully unlikely...Also, aren't we automatically renaming in the case
of conflict?

> * When loading a daap server the user interface is not updated until it
> the network communications are finished.  Ideally, the network stuff
> should be in a separate thread, or at the very least the daap buffer
> parsing stuff should be done incrementally in a timeout.

Yeah...this is going to be important to fix soon actually; consider an
office where you have say 50 people sharing music...  

> * No authentication.

Not high priority in my opinion.

> * Anywhere else there's a 'FIXME'
> 
> So.  Get out there and try it, please!!


Attachment: signature.asc
Description: This is a digitally signed message part



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]