sqlite3_free()

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

sqlite3_free()

D. Richard Hipp
Two SQLite APIs, sqlite3_exec() and sqlite3_mprintf(), return
strings in memory obtained from a malloc-like memory allocator.
The documentation has always said that you need to use sqlite3_free()
in order to free those strings.  But, as it happens, it has
until now worked to call plain old free().

But that might change.  In the latest code in CVS, if you
disregard the documentation and use free() in place of
sqlite3_free(), it will likely lead to a segfault.  It
might still work depending on how you compile.  But a
segfault is the more likely outcome.

So correct code should continue to work fine.  But broken
code that happened to work before might now really break.

I'm hoping that this change will not have too much adverse
impact.  If you think this change might cause excessive
hardship, please let me know (before the next release!) and
we will consider using (suboptimal) alternatives that allow
the older broken code to continue functioning.  If I do not
hear a sufficiently large outcry, the new code will appear
in the next release.

--
D. Richard Hipp   <[hidden email]>

Reply | Threaded
Open this post in threaded view
|

Re: sqlite3_free()

Christian Smith-4
[hidden email] uttered:

> Two SQLite APIs, sqlite3_exec() and sqlite3_mprintf(), return
> strings in memory obtained from a malloc-like memory allocator.
> The documentation has always said that you need to use sqlite3_free()
> in order to free those strings.  But, as it happens, it has
> until now worked to call plain old free().
>
> But that might change.  In the latest code in CVS, if you
> disregard the documentation and use free() in place of
> sqlite3_free(), it will likely lead to a segfault.  It
> might still work depending on how you compile.  But a
> segfault is the more likely outcome.
>
> So correct code should continue to work fine.  But broken
> code that happened to work before might now really break.


My own personal opinion on these coding style issues is if the API
requires special handling of cleanup, then the API should do the cleanup.
Returning an allocated string that requires special cleanup results in a
potentially generic operation now being special cased by the API client.

While it's too late to change now, this puts the client in the unenviable
position of needed to copy the string anyway if the string is required
elsewhere in the client that may not be aware of the special SQLite API
requirements.


>
> I'm hoping that this change will not have too much adverse
> impact.  If you think this change might cause excessive
> hardship, please let me know (before the next release!) and
> we will consider using (suboptimal) alternatives that allow
> the older broken code to continue functioning.  If I do not
> hear a sufficiently large outcry, the new code will appear
> in the next release.


How is free() sub-optimal? IMHO, malloc/free is not something an API
should be trying to optimise other than internally and opaquely to the
API client. You want to block allocate buffers? Fine, do it in SQLite, but
exporting this to the API is the implementation showing through.

If the client wants to do memory checking, then the developer should link
against instrumented malloc/free like valgrind or ElectricFence.

As to the actual change, I guess this is trying to optimise the realloc
case in the future, perhaps? Is this truly a bottleneck? Otherwise, the
current CVS implementation doesn't add anything.

If this seems like a rant, I'm sorry. I just hate the practice of
overriding malloc/free because it makes API specific a generic case.
Memory allocation is something the original C standard library got mostly
right.


>
> --
> D. Richard Hipp   <[hidden email]>
>

Christian


--
     /"\
     \ /    ASCII RIBBON CAMPAIGN - AGAINST HTML MAIL
      X                           - AGAINST MS ATTACHMENTS
     / \
Reply | Threaded
Open this post in threaded view
|

Re: sqlite3_free()

D. Richard Hipp
Christian Smith <[hidden email]> wrote:
>
> My own personal opinion on these coding style issues is if the API
> requires special handling of cleanup, then the API should do the cleanup.
> Returning an allocated string that requires special cleanup results in a
> potentially generic operation now being special cased by the API client.
>

If all the world was Unix, this would work great.  But sadly,
it is not.  We also have to support windows.  See

   http://www.sqlite.org/cvstrac/tktview?tn=444

The sqlite_freemem() API is an old SQLite version 2 API that was
added to work around the fact that memory allocated using malloc()
in a DLL cannot be passed to free() in the main program.

So I do have to provides sqlite3_free() at least, if for no
other purpose than to support windows users.  And notice too
that if you want to your code to be portable to windows, then
you have to use sqlite3_free() to release memory coming back
out of sqlite3_mprintf() or sqlite3_exec().  There appears to
be no way to avoid this due to the limitations of windows DLLs.

The other thing is that since version 3.3.0, SQLite has allowed
an implementation to define its own malloc/free implementation
by overloading the sqlite3OsMalloc() and sqlite3OsFree() interfaces
on the OS-layer.  This is sometimes a useful thing to do.  The
sqlite3_malloc() and sqlite3_free() APIs provide a portable way
to get the application-preferred memory allocator.

The default implementation of sqlite3OsMalloc() is not compatible
with malloc(), however, since it needs to be extended to support
sqlite3AllocationSize() (a capability that is tragically missing
from the standard unix-style malloc()).  

I could continue to provide an sqlite3_free() that is compatible
with free() on unix systems and then provide another set of
routines, sqlite3_osfree(), sqlite3_osmalloc(), sqlite3_osrealloc(),
etc., that provide access to the OS-layer memory allocator.  But
then we would have two separate memory allocation systems which
seems even more confusing than requiring the use of sqlite3_free().
And, notice also that the added complication only allows you to
avoid using sqlite3_free() on Unix platforms - it is of no help
on windows.

--
D. Richard Hipp   <[hidden email]>

Reply | Threaded
Open this post in threaded view
|

Re: sqlite3_free()

Christian Smith-4
[hidden email] uttered:

> Christian Smith <[hidden email]> wrote:
>>
>> My own personal opinion on these coding style issues is if the API
>> requires special handling of cleanup, then the API should do the cleanup.
>> Returning an allocated string that requires special cleanup results in a
>> potentially generic operation now being special cased by the API client.
>>
>
> If all the world was Unix, this would work great.  But sadly,
> it is not.  We also have to support windows.  See
>
>   http://www.sqlite.org/cvstrac/tktview?tn=444
>
> The sqlite_freemem() API is an old SQLite version 2 API that was
> added to work around the fact that memory allocated using malloc()
> in a DLL cannot be passed to free() in the main program.


Yes, of course, Windows sticks it's oar in again. Going back to the
previous DLL discussion, this alone is surely confirmation of why the
Windows DLL system sucks.

My previous rant was really that, just a rant. Given the previous
interface, you must maintain compatibility, and breaking the old use of
free() should be acceptable. My own code is not affected, as I already
used sqlite_freemem (stuck with 2.x for the moment.)

For reference (well, for my reference at least) I believe that returned
memory should be considered static to the database connection, with
subsequent invocations overwriting the previous contents. That way, all
management would be internal to the API, and if the client wants a copy,
he should copy it before the next invocation. This is especially true of
such things as error strings.


>
> --
> D. Richard Hipp   <[hidden email]>
>

Christian


--
     /"\
     \ /    ASCII RIBBON CAMPAIGN - AGAINST HTML MAIL
      X                           - AGAINST MS ATTACHMENTS
     / \
Reply | Threaded
Open this post in threaded view
|

Re: sqlite3_free()

Dennis Cote
Christian Smith wrote:
>
> Yes, of course, Windows sticks it's oar in again. Going back to the
> previous DLL discussion, this alone is surely confirmation of why the
> Windows DLL system sucks.
>
This really has nothing to do with the Windows DLL system. It is simply
the case that the main application and the SQLite library may be
compiled with different compilers that use different runtime libraries
and therefore implement different memory heaps. You can't in general
expect memory that was allocated from one heap by one runtime library
(used by SQLite) to be correctly released to another heap maintained by
a another runtime library used by the application.

Under *nix it is more common, but not required, for applications to link
to one common runtime library.
>
> For reference (well, for my reference at least) I believe that
> returned memory should be considered static to the database
> connection, with subsequent invocations overwriting the previous
> contents. That way, all management would be internal to the API, and
> if the client wants a copy, he should copy it before the next
> invocation. This is especially true of such things as error strings.
>
Ack! No! This leads to non-reentrant code. This is the kind of problem
that the standard asctime() API has. It is much better for the caller to
provide the memory buffer, or have the library dynamically allocate the
buffer and pass it back to the caller. In this case you never have to
worry about some other thread calling the function before your thread
has completed its copy.

Dennis Cote
Reply | Threaded
Open this post in threaded view
|

Re: sqlite3_free()

Christian Smith-4
Dennis Cote uttered:

> Christian Smith wrote:
>>
>> Yes, of course, Windows sticks it's oar in again. Going back to the
>> previous DLL discussion, this alone is surely confirmation of why the
>> Windows DLL system sucks.
>>
> This really has nothing to do with the Windows DLL system. It is simply the
> case that the main application and the SQLite library may be compiled with
> different compilers that use different runtime libraries and therefore
> implement different memory heaps. You can't in general expect memory that was
> allocated from one heap by one runtime library (used by SQLite) to be
> correctly released to another heap maintained by a another runtime library
> used by the application.
>
> Under *nix it is more common, but not required, for applications to link to
> one common runtime library.


Under UNIX it is more common because UNIX provides a runtime system by
default. Windows programs all ship with their own runtime due to sloppy
engineering on MS's part. It harks back to the days when each DLL had it's
own local data segment under Win16. Implementation details from 20 years
ago biting us in the bum even when the Win32 API doesn't have segments!


>>
>> For reference (well, for my reference at least) I believe that returned
>> memory should be considered static to the database connection, with
>> subsequent invocations overwriting the previous contents. That way, all
>> management would be internal to the API, and if the client wants a copy, he
>> should copy it before the next invocation. This is especially true of such
>> things as error strings.
>>
> Ack! No! This leads to non-reentrant code. This is the kind of problem that
> the standard asctime() API has. It is much better for the caller to provide
> the memory buffer, or have the library dynamically allocate the buffer and
> pass it back to the caller. In this case you never have to worry about some
> other thread calling the function before your thread has completed its copy.


Static is probably the wrong word. The string is local to the database
connection, which shouldn't be used by more than one thread without proper
synchronisation.


Anyway, it's not difficult to provide thread local storage. HP-UX's
netdb.h functions (gethostbyname etc.) are fully re-entrant despite
returning 'static' data, for example. Other UNIXs got hamstrung with
various getXbyY_r implementations, with horrible semantics.


>
> Dennis Cote
>


Christian

--
     /"\
     \ /    ASCII RIBBON CAMPAIGN - AGAINST HTML MAIL
      X                           - AGAINST MS ATTACHMENTS
     / \
Reply | Threaded
Open this post in threaded view
|

Re: sqlite3_free()

Andrew Piskorski
On Tue, Jun 27, 2006 at 04:14:37PM +0100, Christian Smith wrote:

> Anyway, it's not difficult to provide thread local storage. HP-UX's
> netdb.h functions (gethostbyname etc.) are fully re-entrant despite
> returning 'static' data, for example. Other UNIXs got hamstrung with
> various getXbyY_r implementations, with horrible semantics.

Well yes, the *_r functions are often pretty ugly to use.  But they
work great if what you want to do is build your own thread local
storage version on top!

I've always assumed there's some good reason for the existence and use
of *_r functions rather than equivalent thread local storage versions,
although I've never been sure just what it is.

--
Andrew Piskorski <[hidden email]>
http://www.piskorski.com/
Reply | Threaded
Open this post in threaded view
|

Re: sqlite3_free()

Christian Smith-4
Andrew Piskorski uttered:

> On Tue, Jun 27, 2006 at 04:14:37PM +0100, Christian Smith wrote:
>
>> Anyway, it's not difficult to provide thread local storage. HP-UX's
>> netdb.h functions (gethostbyname etc.) are fully re-entrant despite
>> returning 'static' data, for example. Other UNIXs got hamstrung with
>> various getXbyY_r implementations, with horrible semantics.
>
> Well yes, the *_r functions are often pretty ugly to use.  But they
> work great if what you want to do is build your own thread local
> storage version on top!
>
> I've always assumed there's some good reason for the existence and use
> of *_r functions rather than equivalent thread local storage versions,
> although I've never been sure just what it is.


Mainly because the _r functions were hacked by lazy types who couldn't be
bothered to use TLS (or TLS wasn't available). The _r functions weren't
particularly well thought out, leaving the client to allocate the storage
(arguably good) without telling the client how big the storage has to
actually be (definitely bad). It is this type of implementation issue that
should be completely hidden from the client, hence my preferred use of TLS
for 'static' buffers managed by the API.

Grr, I'm definitely sounding like I'm ranting now:)


Christian

--
     /"\
     \ /    ASCII RIBBON CAMPAIGN - AGAINST HTML MAIL
      X                           - AGAINST MS ATTACHMENTS
     / \
Reply | Threaded
Open this post in threaded view
|

Re: sqlite3_free()

Kervin L. Pierre
In reply to this post by Dennis Cote
Hello,

--- Dennis Cote <[hidden email]> wrote:

> This really has nothing to do with the Windows DLL
> system. It is simply

Thanks for the explanation.  Wondered what that
bug reporter was talking about :)

There's a lot Windows does wrong, we don't have
to go around making up stuff :)

SQLite could export its memory management
routines as function pointers to the host app.
And have the host app provide their
implementations if desired.  That would solve
this issue amongst others.

Best regards,
Kervin