On Thu, 28.08.14 12:56, Michal Sekletar (msekl...@redhat.com) wrote: > On Tue, Aug 26, 2014 at 08:54:01PM +0200, Lennart Poettering wrote: > > On Mon, 25.08.14 10:02, Michal Sekletar (msekl...@redhat.com) wrote: > > > > > +int label_get_our_label(char **label) { > > > + int r = 0; > > > + char *l = NULL; > > > + > > > +#ifdef HAVE_SELINUX > > > + r = getcon(&l); > > > + if (r < 0) > > > + return r; > > > + > > > + *label = l; > > > +#endif > > > + > > > + return r; > > > +} > > > > Hmm, we shouldn't return success if selinux support is turned off, and > > we don't write anything to *label... This really should return -ENOTSUP > > or so, i figure.... > > Are you suggesting that we should change other functions in label.c as well? > AFAICT all non-void functions from that module implement same pattern > now.
No, I think the calls that can safely be NOPs if selinux support is missing should just become NOPs and return 0, pretending success... But lavel_get_out_label() above is not one of those: if we return 0 when selinux is not available, then the calling code might assume success and use the returned string which is then non-initialized. Hence, in this case we should tell the caller that what he asked for didn't work, anhd return ENOTSIP or so.... > > > +#include "util.h" > > > + > > > +#ifdef HAVE_SELINUX > > > +#include <selinux/selinux.h> > > > +#include <selinux/context.h> > > > +#endif > > > + > > > int label_init(const char *prefix); > > > void label_finish(void); > > > > > > @@ -39,6 +46,8 @@ void label_context_clear(void); > > > void label_free(const char *label); > > > > > > int label_get_create_label_from_exe(const char *exe, char **label); > > > +int label_get_our_label(char **label); > > > +int label_get_child_mls_label(int socket_fd, const char *exec, char > > > **label); > > > > > > int label_mkdir(const char *path, mode_t mode); > > > > > > @@ -49,3 +58,11 @@ int label_apply(const char *path, const char *label); > > > int label_write_one_line_file_atomic(const char *fn, const char *line); > > > int label_write_env_file(const char *fname, char **l); > > > int label_fopen_temporary(const char *path, FILE **_f, char > > > **_temp_path); > > > + > > > +#ifdef HAVE_SELINUX > > > +DEFINE_TRIVIAL_CLEANUP_FUNC(security_context_t, freecon); > > > +DEFINE_TRIVIAL_CLEANUP_FUNC(context_t, context_free); > > > + > > > +#define _cleanup_security_context_free_ _cleanup_(freeconp) > > > +#define _cleanup_context_free_ _cleanup_(context_freep) > > > +#endif > > > > Hmm, wouldn't it suffice to have the latter four lines simply in > > label.c, not in the header files? I'd prefer if we didn't have to > > include the selinux headers from the header file... > > Quick check revealed that we are using for example security_context_t type in > following modules other than label.c: > > src/core/selinux-access.c > src/core/selinux-setup.c > src/journal/journald-server.c > src/journal/journald-stream.c /* now cleanup macros can't be of any use here > */ > src/nspawn/nspawn.c /* here as well */ > > Honestly I don't really care either way. Given that use cases in above modules > for new macros are really minimal (1-2 per module) we can just put those four > lines to C file make use of macros there and leave the rest as is. Hmm, maybe introduce selinux-util.h where we can place these macros, which then can be used in the modules that actually include the selinux headers... What I want to avoid here is that the selinux headers are pulled into every single .c module indirectly... I want that the compiler will still generate errors if somebody calls an selinux API function if somebody forgot to include the header files... Thanks, Lennart -- Lennart Poettering, Red Hat _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel