mt-daapd breaks ourtunes (with fix)

[Shish]

Latest nightly, has an empty database; ourtunes scans all the other (iTunes) users on my net successfully, then gets to my (mt-daapd) server and dies with the error:

Exception in thread "Thread-1" java.lang.NullPointerException

at itunes.client.request.Request.Process(Request.java:169)

at itunes.client.request.Request.(Request.java:71)

at itunes.client.request.LogoutRequest.(LogoutRequest.java:41)

at itunes.client.swing.One2OhMyGod.connectToHost(One2OhMyGod.java:590)

at itunes.client.swing.ConnectWorker.run(ConnectWorker.java:59)

the code in question is

if (data.length==0) {

return;

}

, and the “data” array is initialised in

 try {

url = new URL("http://"+server+":"+port+"/"+requestString);

if (One2OhMyGod.debug)

System.out.println("Processing Request: "+ server+":"+port+"/"+requestString);

URLConnection urlc = url.openConnection();

String hashCode = Hasher.GenerateHash("/" + requestString,1,_status,-1);



One2OhMyGod.debugPrint("Created hash for version " + _status.getItunesHost().getVersion());



urlc.addRequestProperty("Client-DAAP-Validation", hashCode);

urlc.addRequestProperty("Client-DAAP-Access-Index", "1");



DataInputStream in = new DataInputStream(urlc.getInputStream());

int len = urlc.getContentLength();

if (len == -1) {

return;

}

data = new byte[len];

in.readFully(data);

}

Some TCPdumping later, I noticed that mt-daapd doesn’t send a content-length header when it sends an error message, so len = -1, so the “data” array is never created.

My fix is to go to webserver.c, function ws_returnerror, and copy and paste the iTunes special case, and replace the test for “user-agent = iTunes + error code 401” with “user-agent = Java” (+ any error code); ourtunes then works fine.

ED> I was wondering why ws_returnerror would be called without an error actually happening; more looking at debugging logs shows that it’s used for “204: Logout Successful” when ourtunes has finished its scan (which would explain why OT only dies after it’s listed my music)

ED2> Actually, a better fix would be to buffer the error message, and only send it (+ content-length header) when it’s length was known. (And do this for all errors, not just “iTunes” and “Java” user agents).

I also note that mt-daapd uses HTTP keep-alives; is it even possible to combine several keep-alive requests without having content-lengths for each of them? I can’t see how, so it might be a good idea to run through all the code and make sure they’re sent every time, to stop future similar problems…

[rpedde]

ED> I was wondering why ws_returnerror would be called without an error actually happening; more looking at debugging logs shows that it’s used for “204: Logout Successful” when ourtunes has finished its scan (which would explain why OT only dies after it’s listed my music)

Yeah, it’s really ws_return_non_200_code, or something.

ED2> Actually, a better fix would be to buffer the error message, and only send it (+ content-length header) when it’s length was known. (And do this for all errors, not just “iTunes” and “Java” user agents).

Yup, but I don’t want to buffer anything. This has to run on an asus WL-HDD, with only 16mb of ram. I don’t like the idea of buffering that stuff. At some point I’m going to have a quirks table for various user agents, like: this one only accepts keep-alives, and this one uses a single thread for streaming, etc.

In any event, I’d argue it’s breakage on the part of GiT — the headers clearly say Connection-type: Close.

I’ll add the workaround, though.

I also note that mt-daapd uses HTTP keep-alives; is it even possible to combine several keep-alive requests without having content-lengths for each of them? I can’t see how, so it might be a good idea to run through all the code and make sure they’re sent every time, to stop future similar problems…

Only by using chunked encoding, and iTunes (afaik) doesn’t honor chunked encoding. Which is too bad, as it would speed up db enumeration a lot.

As far as running through the code, my side is right. If someone chokes on it, it’s because they don’t comply with the http/1.1 spec. I’ve got plenty of bugs in this codebase, but that isn’t one of them, to my mind.

Oh, and good detective work, btw. Thanks.

— Ron

[czeekh]

Shish, I was glad to see someone else was having this problem. Here, ourTunes still hangs when I tries to connect to my server (1376 nightly). It’s not a HUGE deal (ie, I’m not going to be crushed if this can’t work), but it would be nice if ourTunes could still function with mt-daapd running.

I’m not too familiar with code editing, so feel free to point anything obvious or elementary in what I’m posting. I believe these are the relevant parts of the webserver.c file:

/*

* ws_encoding_hack

*/

int ws_encoding_hack(WS_CONNINFO *pwsc) {

char *user_agent;

int space_as_plus=1;



user_agent=ws_getrequestheader(pwsc, "user-agent");

if(user_agent) {

if(strncasecmp(user_agent,"Java",4) == 0)

space_as_plus=0;

if(strncasecmp(user_agent,"iTunes",6) == 0)

space_as_plus=0;

}

return space_as_plus;

}

AND

int ws_returnerror(WS_CONNINFO *pwsc,int error, char *description) {

char *useragent;



DPRINTF(E_WARN,L_WS,"Thread %d: Entering ws_returnerror (%d: %s)n",

pwsc->threadno,error,description);

ws_writefd(pwsc,"HTTP/1.1 %d %srn",error,description);



/* we'll force a close here unless the user agent is

iTunes, which seems to get pissy about it */

useragent = ws_getarg(&pwsc->request_headers,"User-Agent");

if((useragent) && (strncmp(useragent,"iTunes",6) == 0) && (error == 401)) {

ws_addarg(&pwsc->response_headers,"Connection","keep-alive");

ws_addarg(&pwsc->response_headers,"Content-Length","2");

ws_emitheaders(pwsc);

ws_writefd(pwsc,"rn");

return 0;

}

if((useragent) && (strncmp(useragent,"Java",4) == 0) && (error == 993)) {

ws_addarg(&pwsc->response_headers,"Connection","keep-alive");

ws_addarg(&pwsc->response_headers,"Content-Length","2");

ws_emitheaders(pwsc);

ws_writefd(pwsc,"rn");

return 0;

}

Below that, the seceond piece of code is unaltered. This is on an up-to-date Slackware 10.2 if that’s relevant. What I’m asking is if there is any error in what I’ve done here and if so, how to correct it. Any input is welcome. Thanks

[rpedde]

@czeekh wrote:

I’m not too familiar with code editing, so feel free to point anything obvious or elementary in what I’m posting. I believe these are the relevant parts of the webserver.c file:

Looks okay… could be combined and shortened some, but the proof is in the pudding — does it work?

[czeekh]

It doesn’t work. ourTunes will scan all hosts appearing on the list before/above mine but when it gets to mine, it receives the list of songs and still shows “connected” on the Hosts tab. So, in the (frequently occuring) event my server is at the top of the list, only the songs in my library are shown and no further scanning is done. More accurately, though other hosts are listed, their status is a perpetual “Resolving.” Any ideas? Like I said, if I can’t get this to work, I won’t be heart-broken – but it would be nice. Thanks again.

[rpedde]

@czeekh wrote:

It doesn’t work. ourTunes will scan all hosts appearing on the list before/above mine but when it gets to mine, it receives the list of songs and still shows “connected” on the Hosts tab. So, in the (frequently occuring) event my server is at the top of the list, only the songs in my library are shown and no further scanning is done. More accurately, though other hosts are listed, their status is a perpetual “Resolving.” Any ideas? Like I said, if I can’t get this to work, I won’t be heart-broken – but it would be nice. Thanks again.

Then I’ll add the fix above and see what that does.

— Ron

[czeekh]

I was just curious if you’d gotten a chance to try that code; or if Shish could give any more detail as to how he edited the file. Thanks.

Eric

[rpedde]

@czeekh wrote:

I was just curious if you’d gotten a chance to try that code; or if Shish could give any more detail as to how he edited the file. Thanks.

Eric

Haven’t yet. I’m working on getting a build chain working for automating nightlies. Once I have that, I’ll start back on small bug fixes before I start the db rewrite.

— Ron

[czeekh]

Just thought I’d reply once more in case this thread gets searched sometime down the road. With nightly 1441 my problem with ourTunes hanging is fixed. It looks like this was probably the case after 1433, but I didn’t get a chance to update until 1441 was out. Thanks to both of you.

Eric

[rpedde]

@czeekh wrote:

Just thought I’d reply once more in case this thread gets searched sometime down the road. With nightly 1441 my problem with ourTunes hanging is fixed. It looks like this was probably the case after 1433, but I didn’t get a chance to update until 1441 was out. Thanks to both of you.

Eric

Ahh, good to have feedback. That was all Shish’s fix.

— Ron

[Author]

Posts