Mailing List Archive: exim: users

Returning static strings from dlfunc?

Index | Next | Previous | View Threaded

snabb at epipe

May 6, 2012, 8:44 PM

Post #1 of 7 (260 views)
Permalink

Returning static strings from dlfunc?

Hi all,

I am writing my first dlfunc. My question is: Is it ok to return a
pointer to a static string? I could not find answer to this in the
documentation.

Consider the following example:

#include "local_scan.h"

int foobar(uschar **yield, int argc, uschar *argv[])
{
*yield = US"foobar";
return OK;
}

Is the above correct, or should I write the following instead?

*yield = string_copy(US"foobar");

The first version seems to work fine, but when looking at dlfuncs
written by other people I often see the second alternative which makes a
copy of the original string. Why?

Best Regards,
--
Janne Snabb / EPIPE Communications
snabb [at] epipe - http://epipe.com/

--
## List details at https://lists.exim.org/mailman/listinfo/exim-users
## Exim details at http://www.exim.org/
## Please use the Wiki with this list - http://wiki.exim.org/

exim-users at spodhuis

May 7, 2012, 1:27 AM

Post #2 of 7 (253 views)
Permalink

Re: Returning static strings from dlfunc? [In reply to]

On 2012-05-07 at 10:44 +0700, Janne Snabb wrote:
> I am writing my first dlfunc. My question is: Is it ok to return a
> pointer to a static string? I could not find answer to this in the
> documentation.

It Depends. What is the caller going to do with the result, and might
it try to modify it with strtok(), for instance?

Also, my knowledge of Exim is not encyclopaedic, so there may be some
place where it would break.

> int foobar(uschar **yield, int argc, uschar *argv[])
> {
> *yield = US"foobar";
> return OK;
> }
>
> Is the above correct, or should I write the following instead?

It's not valid C, since you're assigning a const unsigned char * to a
place which is not const, so with warnings turned up you'll get
complaints about assignment dropping const.

> *yield = string_copy(US"foobar");

This is safer. It will make a copy which should be valid for the
lifetime of the current memory allocation pool, and the caller should
have made that a longer-lived pool if necessary for its purposes.
Memory allocation of smaller objects is lighter-weight than malloc, so
you *should* be okay with just doing this.

The benefit of allocation pools is that you can have a pool for a
particular context and, when you finish with that context, throw away
the pool in one go, knowing that anything which still wants a copy past
that context was responsible for keeping a copy of the variable in a
different context with the requisite lifetime. So there's one free()
instead of hundreds or thousands. For small strings, this is a major
win.

So: string_copy() will place the memory in context the caller wants,
will be const-safe, is fairly lightweight and, if something *does*
depend upon being able to modify the string in-place (replacing
whitespace with NUL characters, for instance), then you won't end up
trying to modify contents of a read-only page of memory.

Using string_copy() means that a crash outside your code is probably not
your fault. Go ahead, shift the blame onto the Exim Maintainers and any
problems with the robustness of our APIs. :)

-Phil

--
## List details at https://lists.exim.org/mailman/listinfo/exim-users
## Exim details at http://www.exim.org/
## Please use the Wiki with this list - http://wiki.exim.org/

snabb at epipe

May 7, 2012, 3:22 AM

Post #3 of 7 (251 views)
Permalink

Re: Returning static strings from dlfunc? [In reply to]

On 05/07/2012 03:27 PM, Phil Pennock wrote:
> On 2012-05-07 at 10:44 +0700, Janne Snabb wrote:
>>
>> *yield = string_copy(US"foobar");
>
> This is safer. It will make a copy which should be valid for the
> lifetime of the current memory allocation pool, and the caller should
> have made that a longer-lived pool if necessary for its purposes.

Ok. Thank you for your answer. Unfortunately this is where it gets
complicated. I wanted to avoid this complication.

According to the Exim specification[1]:

The dynamic memory that Exim uses when receiving a message is
automatically recycled if another message is received by the same
process

...and also:

Because it is recycled, the normal dynamic memory cannot be used for
holding data that must be preserved over a number of incoming
messages on the same SMTP connection. However, Exim in fact uses two
pools of dynamic memory; the second one is not recycled, and can be
used for this purpose.

However if my dlfunc is called from acl_smtp_connect the current default
"store_pool" is set to POOL_MAIN and likewise if it is called from
acl_smtp_rcvd.

Should I always manually set "store_pool" to POOL_PERM to make sure that
the allocated memory won't be recycled too early (in case my dlfunc is
called from acl_smtp_connect and the SMTP session delivers several
messages)?

If I always set store_pool to POOL_PERM will my dlfunc cause memory
leaks, or is POOL_PERM memory also recycled but only at the end of the
complete SMTP transaction?

Or is this a bug in Exim? Should Exim take care of setting "store_pool"
to POOL_PERM when running a dlfunc in the connect ACL?

Or should my dlfunc do some magic on its own to determine what
"store_pool" it should use, depending on which ACL is calling it?

[1]
http://www.exim.org/exim-html-current/doc/html/spec_html/ch42.html#SECTmemhanloc

--
Janne Snabb / EPIPE Communications
snabb [at] epipe - http://epipe.com/

--
## List details at https://lists.exim.org/mailman/listinfo/exim-users
## Exim details at http://www.exim.org/
## Please use the Wiki with this list - http://wiki.exim.org/

exim-users at spodhuis

May 7, 2012, 3:37 AM

Post #4 of 7 (249 views)
Permalink

Re: Returning static strings from dlfunc? [In reply to]

On 2012-05-07 at 17:22 +0700, Janne Snabb wrote:
> However if my dlfunc is called from acl_smtp_connect the current default
> "store_pool" is set to POOL_MAIN and likewise if it is called from
> acl_smtp_rcvd.

What are you doing with the result?

If you're setting an $acl_c_* variable, then the pool will be set to
POOL_PERM.

What variable are you seeing be referred to, from another ACL, in a
different generation of the POOL_MAIN contents?

If your library needs to persist internal state across calls, you should
stash the value of store_pool away, set it to POOL_PERM, then reset
store_pool back to the stashed values.

It's possible that the default pool should change for some ACLs, but I
don't have enough context to know what problem you're encountering.

> If I always set store_pool to POOL_PERM will my dlfunc cause memory
> leaks, or is POOL_PERM memory also recycled but only at the end of the
> complete SMTP transaction?

POOL_PERM lasts for the lifetime of the process, but Exim is a forking
server, so each connection is handled in one process, and deliveries get
fork/exec'd. There's a lot of process splitting going on, so if you
care about the lifetime of memory beyond a narrow scope, you need to
handle that carefully.

> Or is this a bug in Exim? Should Exim take care of setting "store_pool"
> to POOL_PERM when running a dlfunc in the connect ACL?

Possibly, if I can see what variable the result is being stored away in
and why it's being referred to later, after the lifetime of the pool
generation it's in.

> Or should my dlfunc do some magic on its own to determine what
> "store_pool" it should use, depending on which ACL is calling it?

Almost certainly not; but it's possible that you should be temporarily
setting the pool to POOL_PERM for your particular use-case, always.

-Phil

--
## List details at https://lists.exim.org/mailman/listinfo/exim-users
## Exim details at http://www.exim.org/
## Please use the Wiki with this list - http://wiki.exim.org/

snabb at epipe

May 7, 2012, 4:57 AM

Post #5 of 7 (256 views)
Permalink

Re: Returning static strings from dlfunc? [In reply to]

On 05/07/2012 05:37 PM, Phil Pennock wrote:
> On 2012-05-07 at 17:22 +0700, Janne Snabb wrote:
>> However if my dlfunc is called from acl_smtp_connect the current default
>> "store_pool" is set to POOL_MAIN and likewise if it is called from
>> acl_smtp_rcvd.
>
> What are you doing with the result?
>
> If you're setting an $acl_c_* variable, then the pool will be set to
> POOL_PERM.

I made now the following test dlfunc:

#include "local_scan.h"

int foobar(uschar **yield, int argc, uschar *argv[])
{
*yield = string_copy(US"foobar");

log_write(0, LOG_MAIN,
"POOL_PERM = %d, POOL_MAIN = %d, store_pool = %d, arg = %s",
POOL_PERM, POOL_MAIN, store_pool, argv[0]);

return OK;
}

My ACLs are set up as follows:

acl_smtp_connect = acl_check_connect
acl_smtp_data = acl_check_data

And the ACLs itself:

acl_check_connect:
warn
set acl_c_foobar = ${dlfunc{/usr/lib/exim4/foobar.so}{foobar}{conn}}

acl_check_data:
warn
set acl_m_foobar = ${dlfunc{/usr/lib/exim4/foobar.so}{foobar}{data}}

This produces the following log entries:

2012-05-07 11:33:12 [7246] POOL_PERM = 1, POOL_MAIN = 0, store_pool = 0,
arg = conn
2012-05-07 11:33:14 [7246] 1SRMBm-0001ss-1w POOL_PERM = 1, POOL_MAIN =
0, store_pool = 0, arg = data

Thus looks like "store_pool" is 0 (POOL_MAIN) in both cases. Exim is not
automatically switching the pool when I set acl_c_* variable from SMTP
connect ACL.

Or maybe I am missing something? I am not referring to acl_c_foobar or
acl_m_foobar anywhere after setting them. I am assuming that Exim does
not realize this and does not optimize it on its own.

My ACLs contain other things as well, but they should be unrelated to
this. I am using Exim 4.76 as distributed by Ubuntu 12.04 x86_64 in
exim4-daemon-heavy package. "exim --version" outputs the following:

Exim version 4.76 #1 built 24-Nov-2011 07:42:08
Copyright (c) University of Cambridge, 1995 - 2007
Berkeley DB: Berkeley DB 5.1.25: (January 28, 2011)
Support for: crypteq iconv() IPv6 PAM Perl Expand_dlfunc GnuTLS
move_frozen_messages Content_Scanning DKIM Old_Demime
Lookups (built-in): lsearch wildlsearch nwildlsearch iplsearch cdb dbm
dbmnz dnsdb dsearch ldap ldapdn ldapm mysql nis nis0 passwd pgsql sqlite
Authenticators: cram_md5 cyrus_sasl dovecot plaintext spa
Routers: accept dnslookup ipliteral iplookup manualroute queryprogram
redirect
Transports: appendfile/maildir/mailstore/mbx autoreply lmtp pipe smtp
Fixed never_users: 0
Size of off_t: 8
Configuration file is /var/lib/exim4/config.autogenerated

> It's possible that the default pool should change for some ACLs, but I
> don't have enough context to know what problem you're encountering.

Looks like this is not happening.

> POOL_PERM lasts for the lifetime of the process, but Exim is a forking
> server, so each connection is handled in one process, and deliveries get
> fork/exec'd.

Ah, yes of course. Then using POOL_PERM in every case would not be such
a big sin... the leaked memory will not get accumulated over time.

>> Or should my dlfunc do some magic on its own to determine what
>> "store_pool" it should use, depending on which ACL is calling it?
>
> Almost certainly not; but it's possible that you should be temporarily
> setting the pool to POOL_PERM for your particular use-case, always.

This sounds sensible given the current situation.

--
Janne Snabb / EPIPE Communications
snabb [at] epipe - http://epipe.com/

--
## List details at https://lists.exim.org/mailman/listinfo/exim-users
## Exim details at http://www.exim.org/
## Please use the Wiki with this list - http://wiki.exim.org/

exim-users at spodhuis

May 7, 2012, 5:04 AM

Post #6 of 7 (253 views)
Permalink

Re: Returning static strings from dlfunc? [In reply to]

On 2012-05-07 at 18:57 +0700, Janne Snabb wrote:
> 2012-05-07 11:33:12 [7246] POOL_PERM = 1, POOL_MAIN = 0, store_pool = 0,
> arg = conn
> 2012-05-07 11:33:14 [7246] 1SRMBm-0001ss-1w POOL_PERM = 1, POOL_MAIN =
> 0, store_pool = 0, arg = data
>
> Thus looks like "store_pool" is 0 (POOL_MAIN) in both cases. Exim is not
> automatically switching the pool when I set acl_c_* variable from SMTP
> connect ACL.
>
> Or maybe I am missing something? I am not referring to acl_c_foobar or
> acl_m_foobar anywhere after setting them. I am assuming that Exim does
> not realize this and does not optimize it on its own.

src/acl.c :

----------------------------8< cut here >8------------------------------
/* Connection variables must persist forever */

case ACLC_SET:
{
int old_pool = store_pool;
if (cb->u.varname[0] == 'c') store_pool = POOL_PERM;
acl_var_create(cb->u.varname)->data.ptr = string_copy(arg);
store_pool = old_pool;
}
break;
----------------------------8< cut here >8------------------------------

So for simple assignment to ACL variables, returning a const string
wouldn't matter, but you don't know that your return result won't be
part of a longer expansion string, doing something else, perhaps even
being passed to another dlfunc doing something else strange.

So I'd continue to return in POOL_MAIN and let it be copied to
POOL_PERM.

> > It's possible that the default pool should change for some ACLs, but I
> > don't have enough context to know what problem you're encountering.
>
> Looks like this is not happening.

Sorry, the "should change" was meant as "we should change it so that it
changes for some ACLs".

-Phil

--
## List details at https://lists.exim.org/mailman/listinfo/exim-users
## Exim details at http://www.exim.org/
## Please use the Wiki with this list - http://wiki.exim.org/

snabb at epipe

May 7, 2012, 5:18 AM

Post #7 of 7 (250 views)
Permalink

Re: Returning static strings from dlfunc? [In reply to]

On 05/07/2012 07:04 PM, Phil Pennock wrote:
> src/acl.c :
>
> ----------------------------8< cut here >8------------------------------
> /* Connection variables must persist forever */
>
> case ACLC_SET:
> {
> int old_pool = store_pool;
> if (cb->u.varname[0] == 'c') store_pool = POOL_PERM;
> acl_var_create(cb->u.varname)->data.ptr = string_copy(arg);
> store_pool = old_pool;
> }
> break;
> ----------------------------8< cut here >8------------------------------

Excellent! I was just trying to find this bit of code in Exim source
tree to see what happens to the variable when I set it.

> So I'd continue to return in POOL_MAIN and let it be copied to
> POOL_PERM.

Yes. Looks like this is the correct solution because Exim makes another
copy of the acl_c_* variable contents and stores it in the correct pool.

Thanks a lot for your help!

--
Janne Snabb / EPIPE Communications
snabb [at] epipe - http://epipe.com/

--
## List details at https://lists.exim.org/mailman/listinfo/exim-users
## Exim details at http://www.exim.org/
## Please use the Wiki with this list - http://wiki.exim.org/

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads