Hello all,
I noticed while reading the docs that several stdlib types -- notably File and DbConn are declared as ptr types. As I understand, this makes them vulnerable to use after free errors.
Would it make sense for the relevant close proc s to take the pointer as a var parameter and set it to nil? Then, files and handles would be truly unusable after being freed.
This could break code because of changes in mutability, but could it be a good objective for the next major release?
Hello,
File and Dbconn point to resources handled by C libraries (File is actually C stdlib's FILE* for the C target), and as such should not be messed up with directly. For files, after closing, the C stdlib will see it doesn't point to an opened file and will set errno etc... (Nim checks this and raises an IOError afterwards). But if you set the pointer to nil, the C stdlib will try to dereference the pointer and the program will crash.
Here is the official SQLite documentation:
The C parameter to sqlite3_close(C) and sqlite3_close_v2(C)
must be either a NULL pointer or an sqlite3 object pointer
obtained from sqlite3_open(), sqlite3_open16(), or sqlite3_open_v2(),
and not previously closed.
Calling sqlite3_close() or sqlite3_close_v2() with
a NULL pointer argument is a harmless no-op.
So the FILE* documentation is similar, but I have a feeling that a double free with sqlite would crash due to the handle probably being a malloc ed region as opposed to a file descriptor...
On the other hand, I think most people will create a wrapper over a database connection.
So for databases specifically (or other lower level opaque handles that aren't OS files) does setting nil make sense?
Normally you don't free any object instance from stdlib otherwise mentioned like freeaddrinfo (from both winlean and posix lib) that need to called after getaddrinfo.
Of course if you use those after calling close, that's your responsibility.
Hey I just had a look at close and it doesn't do any error checking.
The following program results in a memory corruption:
var f = open("hello.txt", fmWrite)
f.write("test\n")
f.close()
f.close()
This leads me to think that I should set the pointer to nil. Closing nil results in a segfault, which is probably(?) less dangerous than double-free corruption. I'm not an expert in that area.
I think sink parameters mentionned in the destructor wiki are aimed at that: https://github.com/nim-lang/Nim/wiki/Destructors#use-after-consume
proc close*(handle: sink FileHandle) = ...
And double close will be a compile-time error because only the last read is a sink.