On 15. sep. 2013 11:42, Josh Cepek wrote: > The attached patch fixes bug #330 which causes a regression from 2.2.x > where valid configs won't work in certain cases when using --chroot. A > more terse overview of the change is presented in the patch itself. > > The additional error checking for files & scripts introduced for 2.3 > don't take into account that the file path changes after the chroot > operation, preventing options_postprocess_filechecks() from checking the > proper paths. >
Hi Josh,
Thanks a lot for your patch! First of all, I fully agree to the what
this patch resolves, and I agree some of code as well. But I'm giving
it a NAK currently.
I'm not fully convinced we're doing it the right way. I find the
complexity a bit too much. But I'm also struggling to instantly see a
better approach. So I just wanted to share my thoughts, and we can
hopefully find a better approach.
What I'm struggling with is that we're passing struct options just to
get access to o->chroot_dir. But I see your approach can seem cleaner
this way than pass a char *chroot_dir.
Further, we change a lot of places (the majority in fact) which does not
depend on chroot at all. That is my biggest issue. So I'm wondering if
we should not touch check_file_access() at all and rather have a
separate check_file_access_chroot() which takes the chroot_dir as an
additional const char * variable. This function will just do your
gc_arena stuff, to add the chroot dir to the file name, and pass that
directly to check_file_access(). And this function will be used those
few places it's really needed to check for files/dirs inside a chroot.
In this way, we simplify some of the code paths and avoid introducing
CHKACC_NOOP and CHKACC_CHROOT, also the set_user_script() has access to
struct options, so it can use check_file_access_chroot() directly too.
Does this sounds reasonable to you?
Then there's a few other minor issues. You declare variables in middle
of code blocks. This works fine reasonable compilers. But some Windows
compilers (Visual Studio, I'm looking at you) will choke on that. So we
must declare all variables at the top of the block.
And there are also several whitespace issues. Please ensure your code
is aligned well through 'git diff', that's an easy way to spot these
issues. An example is like this:
@@ -4017,7 +4046,8 @@ static void
set_user_script (struct options *options,
const char **script,
const char *new_script,
- const char *type)
+ const char *type,
+ int access_flags)
Again, thanks a lot for you patch! I like that it resolves a real issue
we've been seeing, and I hope we can find a good way how to improve this
for full inclusion.
--
kind regards,
David Sommerseth
signature.asc
Description: OpenPGP digital signature
