220611 - [FIX]binhex regression (endianness issue)

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

cvs diff -u0 -r 1.11 -r 1.12 netwerk/streamconv/converters/nsBinHexDecoder.cpp 
-#ifndef XP_MAC
+#if defined(XP_MAC) || defined(XP_MACOSX)
                mOctetBuf.val = PR_ntohl(mOctetBuf.val);

Oops.  Now the endianness of the data is wrong on non-Mac systems (since this
code never runs now).  Not cool at all.

Patch coming up that makes binhex sorta limp again on Linux, though still with
issues.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to undo this insanity

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132333 [details] [diff] [review]
Patch to undo this insanity

biesi, darin, could you review?

Comment 3

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

Comment on attachment 132333 [details] [diff] [review]
Patch to undo this insanity

sure, r=biesi, though I am not sure why we need binhex support on non-mac
systems

Comment 4

[Boris Zbarsky [:bzbarsky]]

We actually _only_ need this on non-mac systems.  On the mac this decoder is not
even built.

The idea here is that on non-mac systems you may want to extract the data fork
from a binhex file so as to be able to actually view the data or something, when
some mac person sends it to you in binhex form.

Comment 5

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

um, so, why do we need this ifdef at all, if the decoder isn't built on mac?

Comment 6

[Darin Fisher]

Comment on attachment 132333 [details] [diff] [review]
Patch to undo this insanity

uhm... would it make more sense to do #ifdef LITTLE_ENDIAN instead?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yes, it would.  Couldn't we also remove the ifdef altogether, since ntohl should
do the right thing no matter what the native endianness is?

Comment 8

[Darin Fisher]

bz: good point.  that does seem better.  using ntohl directly would be even
better since it is probably compiled away on big endian machines.  fwiw, NSPR
uses ntohl internally, so it is available on all platforms that we care about. 
the XP problem might be knowing what header(s) to #include.

Comment 9

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

isn't there PR_ntohl?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to that effect.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132618 [details] [diff] [review]
Patch to that effect.

This is a tiny patch that fixes a months-old regression that causes problems on
both the 1.4 and 1.5 branches.	There're a few bugs that seem to have been
filed on this, but nobody ever looked at them...

The patch just undoes an incorrect change to an #ifndef line (actually, removes
the ifdef altogether).	Very safe, and prevents dataloss -- without this patch,
Mozilla cannot handle binhex attachments to emails on non-Mac systems.

Comment 13

[Scott MacGregor]

changing the subject to something more appropriate and not personal.

Comment 14

[steve bowden]

Still have a problem with binhex encoding on moz 2003100304 Windows (me)

when saving attachment moz keeps the file locked (sharing issue) and does not
decode it properly.

Should this bug be reopened?

below are mime headers etc

Content-Type: multipart/mixed;
boundary="------------030304040303010001080203"

This is a multi-part message in MIME format.
--------------030304040303010001080203
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

text part of email.......

--------------030304040303010001080203
Content-Type: application/mac-binhex40;
name="new.pdf"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="new.pdf"

(This file must be converted with BinHex 4.0)

:"fjPGbj`C'B!8%4')%0"8Nm"!!!!XA-!!!!!4+3P8%4',6%Z-L!0*H,Mcp-J$6B
J-#"[BQS02$`0,daPEQGdD#!h)$!J8Jd[4QPXG'9b)#p'E'&dC84PBfpNC5!02Mi
0Fh4bC@&Y$3T)LCbAZiiPZ3f'Rf$HiB66JGZk5j8DpJ*H1$$3lFK`C(J$SbZaJhe
pRk*%L6qTkKi["MM6C(hL6G3Y2(jqI'[a06p#2Tkrje2bcrqM#mrIMmHhYkHLe1Y
lbq0l*H#)%cKbN!$!N3X"EJ,9ACT3@`H'f))!)J)NYMD"d&fifS&Bh43l%$d"IX3
35jXL!JQ"C)##3*Q!Ma5f'd&kIdb4!3Sl9!"#e4BQ)!af`,fQ'"kr2[0pr2[ji`i
C8I"14X6Lb)"K*3li[drEPdP2a[fUhV),L43*I"ARq-jaqPLPDaB6Lf8RlZ+8KNk
6dKZD2Nd*"*!!%FKhQFV+M-EV'M9PBNj(CmiB4U1DeQ8,c+mXe+4RD-13!"0-qK"
jPSAc$f-E*aeAd#E1P#$1,NChQiMJZ9CY$MQR''k,D4Y2$MQ4ahfJh9S3BCmB8JG
bb4,)d3!9A15SA3`0"jPm8%(+h8PX9RI,ChbrfAM84X@`%RFc2J*c'+G0K!'E+FB
j[R1F1F"bC(%%aV!59CaN0q('Qcad2)Z*aE!6prQR6q*p3p1Rb8!!'B%X!#La,#J

etc etc

Comment 15

[Boris Zbarsky [:bzbarsky]]

Steve,

1)  This was a bug for a particular very specific issue.  There are other issues
    with binhex that this bug did not fix.
2)  I checked in the patch at "2003-10-03 17:55", as you can see from the
    comments.  You are using build 2003100304.  That means your build was built 15
    hours before the patch was checked in.  So you may in fact be seeing this
    problem, still.

Comment 16

[steve bowden]

Boris,
Thanks for your comments. I knew that the fix went in on the 3rd Oct from
Jason's Forum. I downloaded zip file from nightly/latest on 7th Oct assuming
that the fix was in. I then tried again this time from nightly/latest-trunk
(don't know/understand the difference?) on the 8th and mail was able to decode
the binhex(ed) attachment.
Thanks for your help

Comment 17

[steve bowden]

Unless this fix has gone in to the 1.5 builds recently it isn't in the 1.5
release nor 1.4.1 !!!

Do we not need support for binhex attachments in the latest releases?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Steve, please note that I asked for driver approval to land this on the 1.4.2
and 1.5 branches....  Until I get that approval, I can't check in the fix to
those branches.  So no, this fix is not in 1.4.1 or 1.5.

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 132618 [details] [diff] [review]
Patch to that effect.

1.5 shipped. removing obsolete request.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Michael, who handles 1.4.2 approvals, do you know?

Comment 21

[Mike Kaply [:mkaply]]

Comment on attachment 132618 [details] [diff] [review]
Patch to that effect.

me

a=mkaply for 1.4.2

Comment 22

[Boris Zbarsky [:bzbarsky]]

Hmm... http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mozilla1.4 says the
branch is closed while we get 1.4.1 out.  Does that still apply?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Checked in to 1.4.2.