240257 - directory listings show wrong sizes for files > 4 GB

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

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

this bug is distributed over all parts of necko that handle file listings...

(note: if you watn to test the patch I'll attach, you need the patch for bug 
240192 as well)

Comment 1

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

Attachment: patch

Comment 2

[Darin Fisher]

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

>Index: netwerk/streamconv/converters/nsIndexedToHTML.cpp

>+    if (size != LL_INIT(0, -1) &&

does this need to use LL_NE?


>Index: xpfe/components/directory/nsDirectoryViewer.cpp

>+      if (size != LL_INIT(0, -1)) {

same question about LL_NE.


>+        // XXX RDF should support 64 bit integers (bug 240160)

XXX comment with a bug link, very nice :-)


r=darin

Comment 3

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

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

darin: oh, indeed, I'll make that change

Comment 4

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

checked in on trunk with changes made

Comment 5

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

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

it would be nice to get this into 1.7 - this just changes the type of some
variables to PRInt64 instead of PRUint32, so it's pretty low risk.

Comment 6

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to 1.7

Comment 7

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

checked in on 1.7 branch

Comment 8

[Jan Ruzicka]

Have you checked all interactions?
Your patch seems to break the gopher support.

Somehow (-1) becomes just FF FF FF FF (32 bit) 
instead of proper (-1) or FF FF FF FF FF FF FF FF (64 bit).

Instead of unknown value the interface is getting information about files 
of size 4GB each.

Comment 9

[timeless]

[e-mail]: oh come on, if you're going to complain, please provide
steps to reproduce, including a nice url.

here's a testcase:
gopher://seanm.ca/

Comment 10

[Jan Ruzicka]

The actual problem is in the initialization of the instance variables.

http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/nsDirIndex.cpp#47

The mSize was changed to PRInt64 but the initialization is for PRUint32 line #48.

 47 nsDirIndex::nsDirIndex() : mType(TYPE_UNKNOWN),
 48                            mSize((PRUint32)(-1)),
 49                            mLastModified(-1) 

The initialization to an unknown number is used in the gopher presentation.

Comment 11

[Jan Ruzicka]

(In reply to comment #9)
> [e-mail]: oh come on, if you're going to complain, please provide
> steps to reproduce, including a nice url.
> 
> here's a testcase:
> gopher://seanm.ca/

OK then it can be:
gopher://gopher.floodgap.com/

I was too bussy looking for the actual cause. look at comment #10.

Comment 12

[timeless]

Attachment: this compiles

i blame the typecast. if not for that, this error would have been spotted at
compile time.

Comment 13

[timeless]

Comment on attachment 147008 [details] [diff] [review]
this compiles

does this fix the problem?  I don't have a xpcshell testworld for this yet.

Comment 14

[Andrew Hagen]

Re-opening as per Jan Ruckiza's comments.

Comment 15

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

sorry for breaking this :(
timeless's patch does look correct, r=me

Comment 16

[Jan Ruzicka]

Looks correct, but I have some questions.
What is with the LL_INIT(0, -1), what advanages does it have against plain -1 ?

Should all PRInt64 variables be initialized with this macro?

(mLastModified comes to mind) 

Comment 17

[Bradley Baetz (:bbaetz)]

Comment on attachment 147008 [details] [diff] [review]
this compiles

Um, isn't that the same thing? Don't you need LL_INIT(-1,-1) ?

Comment 18

[Bradley Baetz (:bbaetz)]

Do you need to change other places, too, in order to use the LL_ macros? (Does
any OS mozilla supports need the LL_ macros, anyway?)

Comment 19

[timeless]

Comment on attachment 147008 [details] [diff] [review]
this compiles

> What is with the LL_INIT(0, -1),

See http://www.mozilla.org/projects/nspr/reference/html/prlong.html

> what advanages does it have against plain -1 ?

in short, it's portable to platforms where the platform or compiler do not
support 64bit types.

> Should all PRInt64 variables be initialized with this macro?

For initialization, one of LL_MaxInt, LL_MinInt, LL_Zero, LL_INIT, or another
preexisting PRInt64/PRUint64. Otherwise one can assign/initialize by using an
LL_ Arithmetic op.

> (mLastModified comes to mind) 

Please file bugs for any of them, that's really beyond the scope of this bug.

> Um, isn't that the same thing?

No. The reason the preexisting code didn't work is that it had a typecast:
> (PRUint32)(-1)

Which made it an unsigned integer which easily fit into 64 bits.

> Don't you need LL_INIT(-1,-1) ?

No.

> Do you need to change other places, too, in order to use the LL_ macros?

Biesi changed the other places.

jan: could you test the patch and confirm that it works? if it works, please
load http://bugzilla.mozilla.org/attachment.cgi?id=147008&action=edit and
change the ? by your name to a +.

Comment 20

[Jan Ruzicka]

(In reply to comment #19)
> (From update of attachment 147008 [details] [diff] [review])
> > What is with the LL_INIT(0, -1),
> 
> See http://www.mozilla.org/projects/nspr/reference/html/prlong.html
> 
> > what advanages does it have against plain -1 ?
> 
> in short, it's portable to platforms where the platform or compiler do not
> support 64bit types.
> 
 (only platforms having long long.)
[...]
> 
> > Don't you need LL_INIT(-1,-1) ?
> 
> No.
> 
> > Do you need to change other places, too, in order to use the LL_ macros?
> 
> Biesi changed the other places.
> 
> jan: could you test the patch and confirm that it works? if it works, please
> load http://bugzilla.mozilla.org/attachment.cgi?id=147008&action=edit and
> change the ? by your name to a +.
> 

Thank you for the patch and detailed explanation.


When I try to go to edit attachment #147008 [details] [diff] [review] the bugzilla replies that I don't
have a permition to edit it.



As for the LL_INIT - it is defined pretty strange:

 #define LL_INIT(hi, lo)  ((hi ## LL << 32) + lo ## LL) 

this means pretty hard math to get the parts right, because of the sign extension.
LL_INIT (-1,-1) would actually lead exactly oposite result in case of
working 64bit arithmetic. 
It would be:
    FF FF FF FF 00 00 00 00 
+   FF FF FF FF FF FF FF FF
---------------------------
= 1 00 00 00 00 FF FF FF FF

How do you initialize  -3000000000 ?

Comment 21

[Jan Ruzicka]

>  (only platforms having long long.)
taking that back - after carefully reading the file.

Comment 22

[Jan Ruzicka]

looks good to me r=jan.ruzicka

Comment 23

[timeless]

Comment on attachment 147008 [details] [diff] [review]
this compiles

recording r=jan,biesi

sorry about that, i should have known better. i'm going to see about getting
that fixed.

Comment 24

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

> this means pretty hard math to get the parts right

indeed. initially (for my first patch here), I had used LL_INIT(-1, -1), but
that did not give the desired result...

I find it somewhat unlikely that this will work when there is no native int64
type. I blame NSPR.

Comment 25

[Bradley Baetz (:bbaetz)]

*** Bug 241928 has been marked as a duplicate of this bug. ***

Comment 26

[timeless]

Comment on attachment 147008 [details] [diff] [review]
this compiles

mozilla/netwerk/streamconv/converters/nsDirIndex.cpp	1.8

Comment 27

[timeless]

Comment on attachment 147008 [details] [diff] [review]
this compiles

this change is needed for the branch. it's small, reviewed and safe.

Comment 28

[chris hofmann]

Comment on attachment 147008 [details] [diff] [review]
this compiles

a=chofmann for 1.7

Comment 29

[timeless]

Comment on attachment 147008 [details] [diff] [review]
this compiles

mozilla/netwerk/streamconv/converters/nsDirIndex.cpp 	1.5.20.2

Comment 30

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

all fixed.

Comment 31

[benc]

Can someone find the depends bugs in FTP and put them here?
I don't know how to verify this, I don't have a 4GB file handy :)

Comment 32

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

(In reply to comment #31)
> Can someone find the depends bugs in FTP and put them here?

Hm... can't find any. With this patch, filesizes on FTP servers seem to display
fine though for files > 4 GB.

> I don't know how to verify this, I don't have a 4GB file handy :)

apply bug 240192's patch if on linux, and:
dd if=/dev/zero of=large_file seek=4G bs=1 count=1

Comment 33

[Michal Cihar]

*** Bug 244496 has been marked as a duplicate of this bug. ***