Some non-tier1 platforms may lack file truncation...

RESOLVED FIXED

Status

()

Core
Networking: Cache
P1
normal
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: Darin Fisher, Assigned: gordon)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

15 years ago
from the security review notes:

  "Some non-tier1 platforms have the problem that if we cache a file, then go
   see the site again and the file has become smaller, we don't erase the 
   difference. This could result in a situation (example) where I go to
   ecommerce site, view item on sale where default amount to buy is 10, come
   back a week later, page smaller (no longer possible to state how many to
   buy), I buy item, but accidentally by 10 items because the old page
   contained amount information."

we should either make adding PR_Truncate higher priority or simply add a
preprocessor #error message to prevent platforms which lack support for file
truncation from building.  this would help maintainers of the various "broken"
ports.

marking security sensitive... this is probably just dependent on some other bug(s).
(Assignee)

Updated

15 years ago
Priority: -- → P1
This seems minor but we might as well fix it. CCing seawood for help. We should
probably take the path of least resistance and refuse to build if we don't have
a usable truncate().
Is the problem that truncate() passes but doesn't actually truncate the file or
that truncate() always returns an error condition?  Right now, we appear to use
truncate() without any checks so platforms without any truncate() support would
fail anyway.  And do we have a workaround for those platforms with a broken
truncate()?

(Reporter)

Comment 3

15 years ago
seawood: we call ftruncate for XP_UNIX builds.  any non-XP_UNIX builds have
special code to call ftruncate.  if some non-XP_UNIX build is not covered by our
#ifdef's then the files will not be truncated.  no function call, no truncation.

we need to make sure that for all platforms, there is a call to truncate the
file.  what about XP_BEOS?  i'm guessing it picks up the XP_UNIX #ifdef?  what
about other platforms?  thx!
Oh.  I was looking at the truncate call at
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsFileSpecUnix.cpp#364 .  Since
it's the system's truncate, it should have the same behavior as ftruncate (I
think).  Assuming that truncate/ftruncate work correctly if they exist, we can
just add this one liner to configure.in to mandate these functions:

AC_CHECK_FUNCS(truncate ftruncate,,AC_MSG_ERROR([Your operating system does not
support truncate and/or ftruncate. Exiting.]))

BeOS is an anomaly.  It's not a unix variant but it contains a number of POSIX
compliant APIs which are usually associated with unix (eg, it does have
ftruncate & truncate).  BeOS does not set XP_UNIX; it uses the separate XP_BEOS
define.  All of the platforms, expect win32 (which doesn't do feature tests) &
mac classic, should be covered by adding the function check.
(Reporter)

Comment 5

15 years ago
then BEOS definitely has this bug.
(Assignee)

Comment 6

15 years ago
...but it sounds like it should be easy to fix.  Once I add the AC_CHECK_FUNCS
line, I should be able to add '| XP_BEOS' to the XP_UNIX implementation.  Right?
Once you add the configure test, you should replace the XP_UNIX ifdef with a
HAVE_FTRUNCATE ifdef.  For the non-unix platforms (namely win32 & OS/2), should
you be using ftruncate, if it exists, or keep the existing truncate functions?

At this point, it only really sounds like BeOS was the only OS that isn't being
covered by the existing ifdefs so adding the || XP_BEOS would be the quick fix
to avoid adding the configure test.

(Assignee)

Comment 8

15 years ago
What are the issues with adding a configure test?  It seems like that would be
the correct way to go.  Does it kick off a clobber build?
Status: NEW → ASSIGNED
There are no real issues with the configure test except that it may fail for
some non-unix platforms (namely OS/2) which you already cover with the other
ifdefs.   Adding the defines will cause the entire world to rebuild once (but
that's not really a problem).
Sounds like we've got a fix. Gordon, is it ready to be posted here and reviewed?
Created attachment 105314 [details] [diff] [review]
Quick fix
Let's get this reviewed and checked in for 1.3a.
(Reporter)

Comment 13

15 years ago
Comment on attachment 105314 [details] [diff] [review]
Quick fix

sr=darin
Attachment #105314 - Flags: superreview+
Attachment #105314 - Flags: review?(gordon)
(Assignee)

Comment 14

15 years ago
Comment on attachment 105314 [details] [diff] [review]
Quick fix

r=gordon
Attachment #105314 - Flags: review?(gordon) → review+
The quick fix has been checked in.
(Assignee)

Comment 16

15 years ago
Okay, marking this fixed.  We have another bug to cover moving Truncate() into
NSPR. We can rework the config testing then.  See bug 88341.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
adding benc@netscape.com to CC list
Bugs published on the Known-vulnerabilities page long ago, removing confidential
flag.
Group: security
You need to log in before you can comment on or make changes to this bug.