the time in seconds is shifted up and an 8b counter value inserted
You're thinking of the C library's implementation of tmpnam()
there. I'm talking about Squash's roll-you-own equivalent function, which uses time()
that only changes once per second. Behold the awfulness!
The native build of Squash could therefore be significantly improved if it were to use tmpnam()
, since mkstemp()
isn't an option on RISC OS.
it's far worse - it builds a filename in
<Wimp$ScrapDir>
which is seeded on the current time in seconds
It's not quite that bad, the time in seconds is shifted up and an 8b counter value inserted, so you can have 256 temporary files per second without clashing.
In addition to addressing Timothy's points above, this push addresses a few additional issues:
cppcheck
job result has regressed to a failing state, but should revert to a pass once it is merged).,ttt
file extension from the source filespec to the destination filespec, and this wasn't being done.I wasn't aware of that Unix filesystem feature, so thanks for pointing that out.
It looks like this feature was primarily used by Squash to ensure enough disc space was available. This was probably important at the time it was written, with people regularly squashing and unsquashing files on floppy discs! Of course, on a Unix system, if you've run out of disc space, you probably have bigger problems that Squash failing, but I've tried to side-step the issue by blatting non-zero data across the requested file extent.
In theory, yes, we could probably do without a temporary file on non-native builds - however, RISC OS builds are going to continue to require one, so it would require breaking the OS abstraction somewhat to do that, which I'd prefer not to do.
Regarding tmpnam
, looking at the RISC OS implementation of gen_temp_name
, it's far worse - it builds a filename in <Wimp$ScrapDir>
which is seeded on the current time in seconds - there's a very high chance of collisions there if you were to run Squash in two TaskWindows concurrently.
I did consider replacing it with tmpfile
which solves the tmpnam
concurrency issue, and has the advantage of being an ISO function and so could do double duty on RISC OS and non-native builds. But there's a significant disadvantage that it only returns a FILE*
handle and if you close it, it auto-deletes. This would entail gen_temp_name
and lots of other functions to be refactored to pass around file handles instead of filenames, which was a bigger change than I fancied making at the same time as everything else.
So mkstemp
it is, and the collision risk for native builds remains.
As you say, a 4 GiB file size limit is currently baked into the Squash file format, but also into the Squash module SWI interface. Trying to expand this limit is out of scope for this MR. Also, some of these variables are used to refer to core memory sizes (such as arguments to fwrite
) so for those, I still think size_t
is appropriate.
What I've done is to create an extra type to describe file offsets - local to Squash, so that it can be tweaked as and when Squash learns how to support big files. At present it's typedeffed to uint32_t
. I've tried to assign variables to size_t
or file_offset
appropriately, which will hopefully give us a leg up as and when Squash is extended.
Typical Unix filesystems support holes so this is useless and neither prevents fragmentation nor verifies sufficient disc space.
tmpnam
is widely regarded as insecure due to the risk of another process creating a symbolic link of the same name. Glibc generates a link-time warning advising the use of mkstemp
instead.
However do we even need a temporary filename since we are using filetype suffixes?
size_t
is the wrong type the size of a file, files can be larger than 4GB on 32-bit operating systems.
POSIX provides off_t
for file sizes which is signed, however given file format limitations uint32_t
might be more appropriate.
Done.
Done.
tmpnam()
does return /tmp/fileQJMVqP
or similar, so no worried about only having a leafname. You're right that it should have been returning a malloc()
ed block, since the pointer is later free()
d. However, it turns out that the higher-level code prefers to work in-memory rather than using temporary files, and modern machines have so much memory that in practice this code path never gets executed - that's how I didn't hit it earlier. I've fixed it anyway.
I've now enabled GitLab CI, so have run the whole thing through cppcheck
. It found these, plus a few more leaks elsewhere, all now fixed. I must have been having a bad day...
Done.
That's the setting used when you run RiscOS/Env/ROOL/CrossTools.sh
. It's a catch-all for "whatever the cross-compiling host's calling standard is", but is also useful in places like this to distinguish between build rules for native and cross-compiling builds.
It doesn't make sense to me to have Global/FileTypes.h
hanging around when building for non-RISC OS targets, and indeed this is the only cross-compiling tool that cares about anything from HdrSrc
, so it seems rather heavyweight to add it just for this. The reason why Squash cares is because it changes its behaviour (compress vs decompress) depending on the filetype of the argument (or in cross-compiling world, whether the argument filename ends with ,fca
or not).
In an attempt to find a compromise, I've moved the #define
outside the #ifdef __riscos
. This way, when we build for RISC OS, cc
will complain loudly if the definitions mismatch the ones from Global/FileTypes.h
, yet when we build a cross-compiling version, it won't matter that Global/FileTypes.h
is missing.