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).
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()?
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.
then BEOS definitely has this bug.
...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.
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?
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?
Let's get this reviewed and checked in for 1.3a.
Comment on attachment 105314 [details] [diff] [review] Quick fix sr=darin
Comment on attachment 105314 [details] [diff] [review] Quick fix r=gordon
The quick fix has been checked in.
Okay, marking this fixed. We have another bug to cover moving Truncate() into NSPR. We can rework the config testing then. See bug 88341.
adding firstname.lastname@example.org to CC list
Bugs published on the Known-vulnerabilities page long ago, removing confidential flag.