The user.localbase sysctl seems like a poor design, and doesn't exist at the moment. That needs to be resolved before this change is committed.

In D27236#608193, @cem wrote:

The user.localbase sysctl seems like a poor design, and doesn't exist at the moment. That needs to be resolved before this change is committed.

The advantage of the sysctl design (once you fix the not going to into the kernel for all user. sysctl) is (a) sysctl can get this value for shell scripts and (b) you can have different, per-jail values and (c) you can make it a tunable to avoid the recompile the world to change issue. the disadvantage is that we have to fix libc to not use compiled-in user.* values which effectively negates all the usual advantages of a sysctl. This could be avoided by selecting a different sysctl name space to use, though it's a poor fit to any of the others.

The advantage of getconf is that it doesn't require we fix the user namespace in sysctl and there's a program that can be used to retrieve it. However, sysconf() has to get the value from $SOMEWHERE and there's no way to make the value come from a config file to make it per-jail, which gets us back to the 'recompile the world' or at least a part thereof, to change it issue.

The advantage of just the env var is that it's simple and direct. However, setuid programs can't trust env values and there's no good central place to set it (though if there were, it would easily generalize into being jail friendly). This poor fit motivated the addition of sysctl in the first place.

In D27236#608246, @imp wrote:

In D27236#608193, @cem wrote:

The user.localbase sysctl seems like a poor design, and doesn't exist at the moment. That needs to be resolved before this change is committed.

The user.localbase sysctl exists in -CURRENT since SVN rev. 367179.
It has not been merged to -STABLE, yet.

The advantage of the sysctl design (once you fix the not going to into the kernel for all user. sysctl) is (a) sysctl can get this value for shell scripts and (b) you can have different, per-jail values and (c) you can make it a tunable to avoid the recompile the world to change issue. the disadvantage is that we have to fix libc to not use compiled-in user.* values which effectively negates all the usual advantages of a sysctl. This could be avoided by selecting a different sysctl name space to use, though it's a poor fit to any of the others.

I have updated libc to still support the R/O user variables while supporting additional R/W variables like user.localbase in the kernel.
See SVN rev, 367243 for the patch that separates the libc-only and the (optionally) kernel provided variables (overriding a prior version that was not as easily extendable).

The advantage of getconf is that it doesn't require we fix the user namespace in sysctl and there's a program that can be used to retrieve it. However, sysconf() has to get the value from $SOMEWHERE and there's no way to make the value come from a config file to make it per-jail, which gets us back to the 'recompile the world' or at least a part thereof, to change it issue.

There is nothing to be fixed - it has been in -CURRENT for more than 2 weeks.

The advantage of just the env var is that it's simple and direct. However, setuid programs can't trust env values and there's no good central place to set it (though if there were, it would easily generalize into being jail friendly). This poor fit motivated the addition of sysctl in the first place.

The env variable LOCALBASE has been supported for this purpose by a (small) number of programs. The getlocalbase() function simplifies the access to this env variable, allows to set a system-wide (possibly jail-wide) override and returns a sane default to reduce the number of checks that need to be performed by the caller.

In D27236#608551, @hselasky wrote:

Looks good to me. I hope it does not affect the performance of any apps, that are launched frequently.

Yes, I had thought about this but then forgotten to mention it in the comment to that version. This is the only drawback of this approach ...
The getenv() call should be cheap, but the 2 sysctl() invocations call into the kernel.
But compared to all the other start-up overhead I do not think that the effect is relevant in practice.

But you cannot have both, an on-demand invocation and guaranteed single invocation in a threaded process.
An alternative would be to use atomics or locking in an on-demand invocation, but I'd rather not add that complexity.

If you ktrace a regular application starting up you already see a lot of system calls, so I think adding one more is fine.

MIght be we also need a destructor to free the memory, if any, to make valgrind happy.

In D27236#608561, @hselasky wrote:

If you ktrace a regular application starting up you already see a lot of system calls, so I think adding one more is fine.

Except there's many efforts to reduce this. At present, this function is called by a tiny number of binaries, so I'd rather have any overhead be limited to those programs that use it.

honestly, I think we're through the looking glass with this discussion...

In D27236#608562, @hselasky wrote:

MIght be we also need a destructor to free the memory, if any, to make valgrind happy.

I could add a destructor, but this would complicate the code just to make a test tool happy ...

I'd need 2 temporary pointers then, one for getenv() - not to be freed - and one for sysctl.
I could assign NULL to tmppath after assigning to localbase in the getenv case, but this might be hard to understand (and somebody might later think that this assignment was not required, remove it, and make the getenv case crash in the destructor when free is applied to LOCALBASE in the environment).

If there is strong demand, I'll add the destructor.
But I really think an assumed valgrind user ought to be able to understand that this left-over variable is benign ...
(And anyway: It does only affect valgrind runs with non-default user.localbase setting in the kernel ...)

In D27236#608678, @imp wrote:

In D27236#608561, @hselasky wrote:

If you ktrace a regular application starting up you already see a lot of system calls, so I think adding one more is fine.

Except there's many efforts to reduce this. At present, this function is called by a tiny number of binaries, so I'd rather have any overhead be limited to those programs that use it.

+1

We don't want to saddle every program that links libutil with a constructor like this. Instead, construct on demand. This could be relatively trivial: drop the ctor attribute, and instead:

getlocalbase(void)
{
    static bool inited = false;
    if (!inited) {
        initlocalbase();
        inited = true;
    }
    return (foo);
}

That said, I still think this design is broken and a bad idea.

In D27236#608681, @cem wrote:

In D27236#608678, @imp wrote:

In D27236#608561, @hselasky wrote:

If you ktrace a regular application starting up you already see a lot of system calls, so I think adding one more is fine.

Except there's many efforts to reduce this. At present, this function is called by a tiny number of binaries, so I'd rather have any overhead be limited to those programs that use it.

+1

We don't want to saddle every program that links libutil with a constructor like this. Instead, construct on demand. This could be relatively trivial: drop the ctor attribute, and instead:

getlocalbase(void)
{
    static bool inited = false;
    if (!inited) {
        initlocalbase();
        inited = true;
    }
    return (foo);
}

That said, I still think this design is broken and a bad idea.

There's no benefit from optimizing this at all. None. This is called once, maybe twice in a few programs. The added complexity isn't worth the maint hassles, nor the added risk from there being an 'oops' in the code leading to some security hole.

In D27236#608678, @imp wrote:

In D27236#608561, @hselasky wrote:

If you ktrace a regular application starting up you already see a lot of system calls, so I think adding one more is fine.

Except there's many efforts to reduce this. At present, this function is called by a tiny number of binaries, so I'd rather have any overhead be limited to those programs that use it.

This affects only programs that link against libutil, some 200 of 1000 programs in base.

honestly, I think we're through the looking glass with this discussion...

There is either a statically allocated 1KB buffer or 2 system calls at start-up, or we go back to the 2nd version I suggested (with malloc, but before the one with the constructor).

We could easily change the implementation (given the call signature is kept) without problem for programs converted to use getlocalbase().
But I want to get us started using that feature ...

In D27236#608681, @cem wrote:

In D27236#608678, @imp wrote:

In D27236#608561, @hselasky wrote:

If you ktrace a regular application starting up you already see a lot of system calls, so I think adding one more is fine.

Except there's many efforts to reduce this. At present, this function is called by a tiny number of binaries, so I'd rather have any overhead be limited to those programs that use it.

+1

We don't want to saddle every program that links libutil with a constructor like this. Instead, construct on demand. This could be relatively trivial: drop the ctor attribute, and instead:

I had suggested 2 versions that did not rely on a constructor, before.

getlocalbase(void)
{
    static bool inited = false;
    if (!inited) {
        initlocalbase();
        inited = true;
    }
    return (foo);
}

This is similar to my 2nd suggested version.
It is not multi-threading safe (think multiple threads started on different cores at exactly the same time).
But the effect of multiple simultaneous invocations can be limited to a small memory leak, or atomics or locks could be used for a "safe" implementation.
Reducing the window was possible by setting inited to true before calling initlocalbase(), but my prior implementation was simpler.

That said, I still think this design is broken and a bad idea.

Why is it a bad idea?

A version that used a caller-provided buffer has been reverted after a number of iterations to get its details right.
It allowed to strcat to the end of the string returned by getlocalbase(), but only after checking for errors.

This version is trivial to use in programs that already use getenv("LOCALBASE") to retrieve a user-supplied path, and this is a common pattern, see D27237 for the required patches and compare with the reverted once that used the prior function signature of the reverted implementation.