Alright, then I'll resubmit with a fix for the compiler warnings and do the rest of the enhancements as a separate patch set.
On Thu, Mar 29, 2018 at 12:35 PM, Stephen Smalley <s...@tycho.nsa.gov> wrote: > On 03/29/2018 11:48 AM, Yuli Khodorkovskiy wrote: >> >> >> On Thu, Mar 29, 2018 at 9:49 AM, Stephen Smalley <s...@tycho.nsa.gov >> <mailto:s...@tycho.nsa.gov>> wrote: >> >> On 03/28/2018 11:40 PM, Yuli Khodorkovskiy wrote: >> > Keep track of line numbers for each file context in >> > selabel_handle. If an error occurs in selabel_fini(), the >> > line number of an invalid file context is echoed to the user. >> > >> > Signed-off-by: Yuli Khodorkovskiy <ykh...@gmail.com >> <mailto:ykh...@gmail.com>> >> > --- >> > libselinux/src/label.c | 2 +- >> > libselinux/src/label_file.h | 1 + >> > libselinux/src/label_internal.h | 1 + >> > 3 files changed, 3 insertions(+), 1 deletion(-) >> > >> > diff --git a/libselinux/src/label.c b/libselinux/src/label.c >> > index e642a97b..d9a58ce9 100644 >> > --- a/libselinux/src/label.c >> > +++ b/libselinux/src/label.c >> > @@ -143,7 +143,7 @@ static int selabel_fini(struct selabel_handle *rec, >> > struct selabel_lookup_rec *lr, >> > int translating) >> > { >> > - if (compat_validate(rec, lr, rec->spec_file, 0)) >> > + if (compat_validate(rec, lr, rec->spec_file, lr->lineno)) >> > return -1; >> > >> > if (translating && !lr->ctx_trans && >> > diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h >> > index aa576d8e..4780ae48 100644 >> > --- a/libselinux/src/label_file.h >> > +++ b/libselinux/src/label_file.h >> > @@ -472,6 +472,7 @@ static inline int process_line(struct >> selabel_handle *rec, >> > spec_arr[nspec].mode = 0; >> > >> > spec_arr[nspec].lr.ctx_raw = context; >> > + spec_arr[nspec].lr.lineno = lineno; >> > >> > /* >> > * bump data->nspecs to cause closef() to cover it in its free >> > diff --git a/libselinux/src/label_internal.h >> b/libselinux/src/label_internal.h >> > index c55efb75..0e020557 100644 >> > --- a/libselinux/src/label_internal.h >> > +++ b/libselinux/src/label_internal.h >> > @@ -73,6 +73,7 @@ struct selabel_lookup_rec { >> > char * ctx_raw; >> > char * ctx_trans; >> > int validated; >> > + unsigned lineno; >> > }; >> > >> > struct selabel_handle { >> > >> >> I think this is ok, but wanted to double check: does this work correctly >> when file contexts are loaded from >> file_contexts.bin instead? It looks to me as if the lineno will be left >> as 0 in that case and the >> code will handle that correctly. >> >> >> Compiling a file_contexts.bin with sefcontext_compile will give you the line >> number: >> >> sefcontext_compile /etc/selinux/targeted/contexts/files/file_contexts -o >> file_contexts.bin -p /etc/selinux/targeted/policy/policy.31 >> libsepol.sepol_context_from_string: malformed context "test" >> libsepol.sepol_context_from_string: could not construct context from string >> libsepol.context_from_string: could not create context structure >> libsepol.sepol_context_to_sid: could not convert test to sid >> /etc/selinux/targeted/contexts/files/file_contexts: line 6132 has invalid >> context test >> sefcontext_compile: process_file failed >> >> Using file_contexts.bin for relabeling that I generated with no validation >> will not report a line number: >> >> restorecon: /etc/selinux/targeted/contexts/files/file_contexts: has invalid >> context test >> >> I'll see if I can associate the line number with each regex in >> sefcontext_compile. > > That's fine if you want to do it but not necessary for these patches to be > merged; I just wanted to ensure that we don't have garbage output as a result > of these patches. I'd do it as a separate patch regardless since it likely > requires a new binary format version for the file_contexts.bin files. > >> The other question is whether we correctly report the file name when the >> entry comes from a file other >> than file_contexts itself, e.g. file_contexts.local, .homedirs, ... Not >> your bug if we don't but wondered. >> >> >> It is reporting the line number correctly, but the filename is incorrect. >> I'll update this. > > Again, not strictly necessary for these patches since you didn't introduce > the bug, and probably ought to be its own separate patch. >
