236425 - On 64bit platforms, IDN doesn't work

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jungshik Shin]

This problem was reported in SuSE Linux bugzilla.

x86_64 patch is not yet committed so that this may not be a legitimate bug.
However, there may be a silimar problem on other 64bit platforms if this bug is
caused by an unknown-dependency on the ALU size somewhere in necko code.

Comment 1

[Wolfgang Rosenauer [:wolfiR]]

AFAIK 1.4.x should work for IDN names, too. If that's the case it doesn't work
on s390x. I still have to check for IA64.

Comment 2

[Wolfgang Rosenauer [:wolfiR]]

it's also broken for moz 1.6 on IA64

Comment 3

[Jungshik Shin]

Thanks for trying it out on other 64bit platforms. I'm changing the summary line
accordingly.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

could you give a more detailed problem description?

Comment 5

[Wolfgang Rosenauer [:wolfiR]]

IDN domains are simply not found (... could not be found. Please check the name...).

network.enableIDN is true on all platforms but on all 64 bit platforms it seems
to do something strange.
I will try to get some packet log what's done in the background.

Comment 6

[Wolfgang Rosenauer [:wolfiR]]

Test-URL was http://björn.j3e.de

on i386 the DNS request looks like:
xn--bjrn-6qa.j3e.de (correct)

on IA64 and the other 64bit platforms it looks like:
bj%C3%B6m.j3e.de

(captured by tcpdump)

Comment 7

[Jungshik Shin]

(In reply to comment #6)
> Test-URL was http://björn.j3e.de
> on i386 the DNS request looks like:
> xn--bjrn-6qa.j3e.de (correct)
> on IA64 and the other 64bit platforms it looks like:
> bj%C3%B6m.j3e.de

Seems like IsASCII (or something like that) is not 64bit-clean. 

Comment 8

[Jungshik Shin]

Wolfgang, can you add a couple of diagnostic printf statements to 

netwerk/dns/src/nsDNSService2.cpp
and
network/dns/src/nsIDNSService.cpp 

and see what's going on? 

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #6)
> on IA64 and the other 64bit platforms it looks like:
> bj%C3%B6m.j3e.de

maybe a bug in nsEscape.cpp?

Comment 10

[Wolfgang Rosenauer [:wolfiR]]

unfortunately I'm not really successful in adding some debug stuff because
all the string handling in Mozilla is really strange for me.

Comment 11

[Jungshik Shin]

Can you try this in nsDNSService2.cpp? You can do similarly in nsIDNSService.cpp.

374     nsresult rv;
375     nsCAutoString hostACE;
        printf ("%s 1> hostname: %s, IsASCII : %c\n", 
                __FILE__, PromiseFlatCString(hostname).get(),
                IsASCII(hostname) ? 'T' : 'F'); 
376     if (idn && !IsASCII(hostname)) {
377         if (NS_SUCCEEDED(idn->ConvertUTF8toACE(hostname, hostACE)))
378             hostPtr = &hostACE;
379     }

        printf ("% 2> hostname: %s, hostACE: %s hostPtr: %s\n", 
                __FILE__, PromiseFlatCString(hostname).get(),
                hostACE.get(), PromiseFlatCString(*hostPtr).get()); 
                

Comment 12

[Wolfgang Rosenauer [:wolfiR]]

thanks.

for www.öko.de it results in

nsDNSService2.cpp 1> hostname: www.www.%C3%B6ko.de, IsASCII : T
% 2> hostname: nsDNSService2.cpp, hostACE: www.www.%C3%B6ko.de hostPtr:

I still have to check in nsIDNService

Comment 13

[Wolfgang Rosenauer [:wolfiR]]

sorry, the patch was broken (missing s)
The real output should be:
nsDNSService2.cpp 1> hostname: www.www.%C3%B6ko.de, IsASCII : T
nsDNSService2.cpp 2> hostname: www.www.%C3%B6ko.de, hostACE:  hostPtr:
www.www.%C3%B6ko.de

IsASCII shouldn't be true here, should it?

Comment 14

[Darin Fisher]

the problem appears to be that someone upstream is escaping the hostname when
they shouldn't be.  the DNS resolver should not be seeing %-escaped hostnames.

Comment 15

[Jungshik Shin]

My change in bug 228176 m...ight be responsible for this problem although not
very likely. I added esc_SkipControl (PR_BIT(15)) in the patch for the bug, but
I did not change the data type of 'flag' (in NS_*EscapeURL) to one that can
store 2^15. 'flag' is currently short (signed). 

Anyway, just in case, Wolfgang, can you try after removing my patch there? 

Comment 16

[Wolfgang Rosenauer [:wolfiR]]

sorry, had no time to test yet but I don't think that it is this patch.
We saw the very same behaviour in 1.4 and 1.5 in the past.

Comment 17

[Jungshik Shin]

Thanks. If you saw the behavior in 1.5, my patch is definitely not to blame. 
(1.4.0 and 1.5 don't have my patch while 1.4.1 and 1.6 have)

I'll try to build a mozilla on AIX 5 to see what's wrong. I have to figure out
configuration options for a 64bit build on AIX5 (Power4).

Comment 18

[Wolfgang Rosenauer [:wolfiR]]

Jungshik, can I help somehow? I can't fix it because my knowledge of the inner
mozilla structures is "tiny" at this time. But I could make tests on 64bit 
platforms.

Comment 19

[Wolfgang Rosenauer [:wolfiR]]

any news here?

Mozilla is a real pain on 64 bit platforms. So we (SUSE) are likely to drop
the 64bit version (at least for AMD64) from our products. That wouldn't be
nice but we have much problems with that builds besides this "minor" problem.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I had a look at this. On www.öko.de, the call to idn_nameprep_map at
nsIDNSService.cpp:411 is failing with "idn_invalid_codepoint". I'll check in there.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, so this code is the problem.

 idn_err = idn_nameprep_map(mNamePrepHandle, (const unsigned long*) ucs4Buf,
		                     (unsigned long*) namePrepBuf, kMaxDNSNodeLen * 3);

ucs4Buf is an array of PRInt32s. unsigned long is 64 bits. "oops"

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think the IDN code should be using unsigned long at all. They should be
using a 32-bit type. Anyway, I'd appreciate someone else figuring out how to fix
this as I don't know the code at all.

Comment 23

[Jungshik Shin]

Thanks for figuring that out. (I haven't yet managed to build a 64bit binary on
AIX).
That code was taken from the IDNkit (of JPNIC). I'll take a look. 

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Any progress here, Jungshik?

Comment 25

[Jungshik Shin]

Attachment: patch

Roc, can you try this on 64-bit platforms? I replaced 'unsigned long' with
PRUint32 and 'unsigned short' with 'PRUint16' in netwerk/dns/src.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually I won't be able to test it for a couple of weeks :-)

Comment 27

[Jungshik Shin]

Wolfgang, can you test it on AMD64 and IA64? 

Comment 28

[Wolfgang Rosenauer [:wolfiR]]

just tested the patch on Firefox 1.0.4-x86-64 and it works now. Thanks.
I could try IA64 and s390x next week if needed.

Comment 29

[Jungshik Shin]

Comment on attachment 185013 [details] [diff] [review]
patch

This patch is like doing "perl -pi -e 's/unsigned long/PRUint32' *" and "perl
-pi -e 's/unsigned short/PRUint16' *" in netwerk/dns/src. (and including
'prtypes.h'). We'd better write to the authors of this code (JPNIC) about
64-bit safeness. In the meantime, this should work for us.

Comment 30

[Jungshik Shin]

Comment on attachment 185013 [details] [diff] [review]
patch

asking for sr.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 185013 [details] [diff] [review]
patch

sr=bzbarsky.

Do we have a bug for the 15-bit issue that was mentioned earlier in the bug?  I
know timeless ran into it too and was going to make nsEscape use a 32-bit int
for the bitfield; did that happen?

Comment 32

[Darin Fisher]

> know timeless ran into it too and was going to make nsEscape use a 32-bit int
> for the bitfield; did that happen?

Yes, it did happen for the escape routines -- at least I recall reviewing the patch.

Comment 33

[Jungshik Shin]

Comment on attachment 185013 [details] [diff] [review]
patch

Thanks for r/sr.
Asking for a to the trunk. This patch doesn't do anything on 32-bit
architectures (because PRUint32/16 are unsigned long/short there), but fixes
the IDN problem on 64-bit architectures. 

Btw, 15-th bit issue was fixed in bug 292908.

Comment 34

[chris hofmann]

Comment on attachment 185013 [details] [diff] [review]
patch

a=chofmann

Comment 35

[Jungshik Shin]

sorry for bug spam. I pressed 'commit' button by mistake without logging
anything. Fix checked into the trunk.