Eliminate use of mktemp -- use mkstemp instead #43
Conversation
This reverts commit 0d1af3b.
This reverts commit a4e131b.
The use of mktemp() results in an unsuppressable warning message on certain compilers, including gcc. This causes problems for projects which have rules regarding warning-free compiles. Use mkstemp instead. The problem with mkstemp is that it opens the file instead of just returning a unique file name. However, as a side effect it does change the template to a unique file name. This can be used similarly to the return value from mktemp.
|
Why not stay with mktemp on MSVC as e.g. here: tkelman/graphviz@acdbce8 ? |
|
That is a valid alternative. To my eye, it dirties up the actual source code more with all the ifdefs in the code versus an ifdef up at the top in the "preamble" code. Perhaps a better fix than either would be to encapsulate the mkstemp/mktemp changes in a static function. The function would return the filename that should be used in the Mat_CreateVer call. This would hide all ifdefs at the top and eliminate confusion as to why mkstemp() was being used just for the side-effect of changing the filename template. |
| @@ -883,7 +885,8 @@ Mat_VarDelete(mat_t *mat, const char *name) | |||
| break; | |||
| } | |||
|
|
|||
| tmp = Mat_CreateVer(tmp_name,mat->header,mat_file_ver); | |||
| close(fd); | |||
| tmp = Mat_CreateVer(temp,mat->header,mat_file_ver); | |||
There was a problem hiding this comment.
This introduces a race condition: After closing the file descriptor fd of the temporary file temp might no longer be a unique temporary filename. Thus a proper fix also requires replacing all fwrite calls by write (which is some effort and modifies/adds the API functions like Mat_CreateVer).
| int fd = mkstemp(temp); | ||
| if (fd > 0) { | ||
| close(fd); | ||
| return temp; |
There was a problem hiding this comment.
This introduces a race condition: After closing the file descriptor fd of the temporary file temp might no longer be a unique temporary filename. Thus a proper fix also requires replacing all fwrite calls by write (which is some effort and changes the existing API functions like Mat_CreateVer).
There was a problem hiding this comment.
True, but I guess I was going for good enough vs perfect. The current pull request eliminates the compiler warnings from mktemp and is at least as good with regards to security and race conditions.
There was a problem hiding this comment.
Note that this is an old issue: https://sourceforge.net/p/matio/bugs/10/
I already considered to fix it but want to do it the perfect way.
There was a problem hiding this comment.
Not sure that the fwrite vs write is the perfect fix either since I don't think it will work with the hdf5-based file formats will it?
There was a problem hiding this comment.
Just saw http://stackoverflow.com/q/1941464. ANSI C compatibility also is important (as I was told at https://trac.modelica.org/Modelica/ticket/1258).
|
Changes Unknown when pulling 62ed547 on gsjaardema:master into * on tbeu:master*. |
|
Changes Unknown when pulling c1e5cb6 on gsjaardema:master into * on tbeu:master*. |
|
Changes Unknown when pulling 62ed547 on gsjaardema:master into * on tbeu:master*. |
|
Changes Unknown when pulling 62ed547 on gsjaardema:master into * on tbeu:master*. |
tbeu
force-pushed
the
master
branch
4 times, most recently
from
68de3b3
to
887adee
Compare
|
Without the dependency on the hdf5 library I figured out a secure temp file creation in Mat_VarDelete using tmpfile (and thus avoiding the file name argument). However, as long as the hdf5 library does not support any secure temp file creation and management, I will not touch it. (HDF5 issue number is HDFFV-8703 from 2014.) So, for now, I close this with wontfix. |
The use of mktemp() results in an unsuppressable warning message
on certain compilers, including gcc. This causes problems for
projects which have rules regarding warning-free compiles.
Use mkstemp instead. The problem with mkstemp is that it opens the
file instead of just returning a unique file name. However, as a
side effect it does change the template to a unique file name.
This can be used similarly to the return value from mktemp.