| Message ID | 1473981238-11051-1-git-send-email-william.c.roberts@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 09/15/2016 07:13 PM, william.c.roberts@intel.com wrote: > From: William Roberts <william.c.roberts@intel.com> > > patch 5e15a52aaa cleans up the process_file() but introduced > a bug. If the binary file cannot be opened, always attempt > to fall back to the textual file, this was not occurring. > > The logic should be: > 1. Open the newest file based on base path + suffix vs > base_path + suffix + ".bin". > 2. If anything fails, attempt base_path + suffix. > > In the case that the file_contexts was the newest file and > used for processing fails, it will attempt the same failure > recovery, which will fail. It was decided to keep it this > was for simplicity. I don't like the approach. What we want is: - if .bin file exists and is not older, try to load it, - if any of the above fails (i.e. .bin file does not exist, is older, or cannot be loaded for any reason), then load the text file. We shouldn't try loading the text file twice. Also, attached is checkpatch output for your patch. Please fix. > > Signed-off-by: William Roberts <william.c.roberts@intel.com> > --- > libselinux/src/label_file.c | 41 +++++++++++++++++++++++++++-------------- > 1 file changed, 27 insertions(+), 14 deletions(-) > > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c > index 9faecdb..0dee059 100644 > --- a/libselinux/src/label_file.c > +++ b/libselinux/src/label_file.c > @@ -447,7 +447,7 @@ static bool fcontext_is_binary(FILE *fp) > #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) > > static FILE *open_file(const char *path, const char *suffix, > - char *save_path, size_t len, struct stat *sb) > + char *save_path, size_t len, struct stat *sb, bool force_text) > { > unsigned int i; > int rc; > @@ -469,7 +469,11 @@ static FILE *open_file(const char *path, const char *suffix, > return NULL; > } > > - for (i = 0; i < ARRAY_SIZE(fdetails); i++) { > + size_t array_size = ARRAY_SIZE(fdetails); > + if (force_text) > + array_size--; > + > + for (i = 0; i < array_size; i++) { > > /* This handles the case if suffix is null */ > path = rolling_append(stack_path, fdetails[i].suffix, > @@ -515,24 +519,33 @@ static int process_file(const char *path, const char *suffix, > const char *prefix, struct selabel_digest *digest) > { > int rc; > + unsigned int i; > struct stat sb; > FILE *fp = NULL; > char found_path[PATH_MAX]; > > - fp = open_file(path, suffix, found_path, sizeof(found_path), &sb); > - if (fp == NULL) > - return -1; > + /* > + * first path open the newest modified file, if it fails, the second > + * pass falls through to the plain text file. > + */ > + for(i=0; i < 2; i++) { > + fp = open_file(path, suffix, found_path, sizeof(found_path), &sb, > + i > 0); > + if (fp == NULL) > + return -1; > > - rc = fcontext_is_binary(fp) ? > - load_mmap(fp, sb.st_size, rec, found_path) : > - process_text_file(fp, prefix, rec, found_path); > - if (rc < 0) > - goto out; > + rc = fcontext_is_binary(fp) ? > + load_mmap(fp, sb.st_size, rec, found_path) : > + process_text_file(fp, prefix, rec, found_path); > + if (!rc) > + rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path); > > - rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path); > -out: > - fclose(fp); > - return rc; > + fclose(fp); > + > + if(!rc) > + return 0; > + } > + return -1; > } > > static void closef(struct selabel_handle *rec); > WARNING: line over 80 characters #84: FILE: libselinux/src/label_file.c:450: + char *save_path, size_t len, struct stat *sb, bool force_text) WARNING: Missing a blank line after declarations #94: FILE: libselinux/src/label_file.c:473: + size_t array_size = ARRAY_SIZE(fdetails); + if (force_text) ERROR: spaces required around that '=' (ctx:VxV) #117: FILE: libselinux/src/label_file.c:531: + for(i=0; i < 2; i++) { ^ ERROR: space required before the open parenthesis '(' #117: FILE: libselinux/src/label_file.c:531: + for(i=0; i < 2; i++) { WARNING: line over 80 characters #118: FILE: libselinux/src/label_file.c:532: + fp = open_file(path, suffix, found_path, sizeof(found_path), &sb, WARNING: line over 80 characters #132: FILE: libselinux/src/label_file.c:541: + rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path); ERROR: space required before the open parenthesis '(' #140: FILE: libselinux/src/label_file.c:545: + if(!rc) total: 3 errors, 4 warnings, 65 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. ../[PATCH v3] libselinux: correct error path to always try text.eml has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.
On Fri, Sep 16, 2016 at 7:30 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 09/15/2016 07:13 PM, william.c.roberts@intel.com wrote: >> From: William Roberts <william.c.roberts@intel.com> >> >> patch 5e15a52aaa cleans up the process_file() but introduced >> a bug. If the binary file cannot be opened, always attempt >> to fall back to the textual file, this was not occurring. >> >> The logic should be: >> 1. Open the newest file based on base path + suffix vs >> base_path + suffix + ".bin". >> 2. If anything fails, attempt base_path + suffix. >> >> In the case that the file_contexts was the newest file and >> used for processing fails, it will attempt the same failure >> recovery, which will fail. It was decided to keep it this >> was for simplicity. > > I don't like the approach. What we want is: > - if .bin file exists and is not older, try to load it, > - if any of the above fails (i.e. .bin file does not exist, is older, or > cannot be loaded for any reason), then load the text file. Functionally that's what it does, you just don't like the implementation. > > We shouldn't try loading the text file twice. It get's a lot more clunky if we go that route, ill take another stab and send something out this afternoon. > > Also, attached is checkpatch output for your patch. Please fix. I tried using checkpatch yesterday, but Ill have to DL the whole linux tree to get it to work, do you know if theirs a stand-alone version anywhere? I am almost out of disk space at the moment, waiting on a new drive. > >> >> Signed-off-by: William Roberts <william.c.roberts@intel.com> >> --- >> libselinux/src/label_file.c | 41 +++++++++++++++++++++++++++-------------- >> 1 file changed, 27 insertions(+), 14 deletions(-) >> >> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c >> index 9faecdb..0dee059 100644 >> --- a/libselinux/src/label_file.c >> +++ b/libselinux/src/label_file.c >> @@ -447,7 +447,7 @@ static bool fcontext_is_binary(FILE *fp) >> #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) >> >> static FILE *open_file(const char *path, const char *suffix, >> - char *save_path, size_t len, struct stat *sb) >> + char *save_path, size_t len, struct stat *sb, bool force_text) >> { >> unsigned int i; >> int rc; >> @@ -469,7 +469,11 @@ static FILE *open_file(const char *path, const char *suffix, >> return NULL; >> } >> >> - for (i = 0; i < ARRAY_SIZE(fdetails); i++) { >> + size_t array_size = ARRAY_SIZE(fdetails); >> + if (force_text) >> + array_size--; >> + >> + for (i = 0; i < array_size; i++) { >> >> /* This handles the case if suffix is null */ >> path = rolling_append(stack_path, fdetails[i].suffix, >> @@ -515,24 +519,33 @@ static int process_file(const char *path, const char *suffix, >> const char *prefix, struct selabel_digest *digest) >> { >> int rc; >> + unsigned int i; >> struct stat sb; >> FILE *fp = NULL; >> char found_path[PATH_MAX]; >> >> - fp = open_file(path, suffix, found_path, sizeof(found_path), &sb); >> - if (fp == NULL) >> - return -1; >> + /* >> + * first path open the newest modified file, if it fails, the second >> + * pass falls through to the plain text file. >> + */ >> + for(i=0; i < 2; i++) { >> + fp = open_file(path, suffix, found_path, sizeof(found_path), &sb, >> + i > 0); >> + if (fp == NULL) >> + return -1; >> >> - rc = fcontext_is_binary(fp) ? >> - load_mmap(fp, sb.st_size, rec, found_path) : >> - process_text_file(fp, prefix, rec, found_path); >> - if (rc < 0) >> - goto out; >> + rc = fcontext_is_binary(fp) ? >> + load_mmap(fp, sb.st_size, rec, found_path) : >> + process_text_file(fp, prefix, rec, found_path); >> + if (!rc) >> + rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path); >> >> - rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path); >> -out: >> - fclose(fp); >> - return rc; >> + fclose(fp); >> + >> + if(!rc) >> + return 0; >> + } >> + return -1; >> } >> >> static void closef(struct selabel_handle *rec); >> > > > _______________________________________________ > Seandroid-list mailing list > Seandroid-list@tycho.nsa.gov > To unsubscribe, send email to Seandroid-list-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Seandroid-list-request@tycho.nsa.gov.
On 09/16/2016 10:30 AM, Stephen Smalley wrote: > On 09/15/2016 07:13 PM, william.c.roberts@intel.com wrote: >> From: William Roberts <william.c.roberts@intel.com> >> >> patch 5e15a52aaa cleans up the process_file() but introduced >> a bug. If the binary file cannot be opened, always attempt >> to fall back to the textual file, this was not occurring. >> >> The logic should be: >> 1. Open the newest file based on base path + suffix vs >> base_path + suffix + ".bin". >> 2. If anything fails, attempt base_path + suffix. >> >> In the case that the file_contexts was the newest file and >> used for processing fails, it will attempt the same failure >> recovery, which will fail. It was decided to keep it this >> was for simplicity. > > I don't like the approach. What we want is: > - if .bin file exists and is not older, try to load it, > - if any of the above fails (i.e. .bin file does not exist, is older, or > cannot be loaded for any reason), then load the text file. > > We shouldn't try loading the text file twice. > > Also, attached is checkpatch output for your patch. Please fix. Also, there is a further wrinkle: Android passes in file_contexts.bin as the SELABEL_OPT_PATH, so that is the base path. Under the old logic (before your original clean up patch), we would open that file, detect that it is binary, and then load it. Under the current logic, we'll open file_contexts.bin, then try to open file_contexts.bin.bin (which will fail), and then use the first one. Wondering if we just need to revert. > >> >> Signed-off-by: William Roberts <william.c.roberts@intel.com> >> --- >> libselinux/src/label_file.c | 41 +++++++++++++++++++++++++++-------------- >> 1 file changed, 27 insertions(+), 14 deletions(-) >> >> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c >> index 9faecdb..0dee059 100644 >> --- a/libselinux/src/label_file.c >> +++ b/libselinux/src/label_file.c >> @@ -447,7 +447,7 @@ static bool fcontext_is_binary(FILE *fp) >> #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) >> >> static FILE *open_file(const char *path, const char *suffix, >> - char *save_path, size_t len, struct stat *sb) >> + char *save_path, size_t len, struct stat *sb, bool force_text) >> { >> unsigned int i; >> int rc; >> @@ -469,7 +469,11 @@ static FILE *open_file(const char *path, const char *suffix, >> return NULL; >> } >> >> - for (i = 0; i < ARRAY_SIZE(fdetails); i++) { >> + size_t array_size = ARRAY_SIZE(fdetails); >> + if (force_text) >> + array_size--; >> + >> + for (i = 0; i < array_size; i++) { >> >> /* This handles the case if suffix is null */ >> path = rolling_append(stack_path, fdetails[i].suffix, >> @@ -515,24 +519,33 @@ static int process_file(const char *path, const char *suffix, >> const char *prefix, struct selabel_digest *digest) >> { >> int rc; >> + unsigned int i; >> struct stat sb; >> FILE *fp = NULL; >> char found_path[PATH_MAX]; >> >> - fp = open_file(path, suffix, found_path, sizeof(found_path), &sb); >> - if (fp == NULL) >> - return -1; >> + /* >> + * first path open the newest modified file, if it fails, the second >> + * pass falls through to the plain text file. >> + */ >> + for(i=0; i < 2; i++) { >> + fp = open_file(path, suffix, found_path, sizeof(found_path), &sb, >> + i > 0); >> + if (fp == NULL) >> + return -1; >> >> - rc = fcontext_is_binary(fp) ? >> - load_mmap(fp, sb.st_size, rec, found_path) : >> - process_text_file(fp, prefix, rec, found_path); >> - if (rc < 0) >> - goto out; >> + rc = fcontext_is_binary(fp) ? >> + load_mmap(fp, sb.st_size, rec, found_path) : >> + process_text_file(fp, prefix, rec, found_path); >> + if (!rc) >> + rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path); >> >> - rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path); >> -out: >> - fclose(fp); >> - return rc; >> + fclose(fp); >> + >> + if(!rc) >> + return 0; >> + } >> + return -1; >> } >> >> static void closef(struct selabel_handle *rec); >> > > > > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. >
On Fri, Sep 16, 2016 at 7:38 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 09/16/2016 10:30 AM, Stephen Smalley wrote: >> On 09/15/2016 07:13 PM, william.c.roberts@intel.com wrote: >>> From: William Roberts <william.c.roberts@intel.com> >>> >>> patch 5e15a52aaa cleans up the process_file() but introduced >>> a bug. If the binary file cannot be opened, always attempt >>> to fall back to the textual file, this was not occurring. >>> >>> The logic should be: >>> 1. Open the newest file based on base path + suffix vs >>> base_path + suffix + ".bin". >>> 2. If anything fails, attempt base_path + suffix. >>> >>> In the case that the file_contexts was the newest file and >>> used for processing fails, it will attempt the same failure >>> recovery, which will fail. It was decided to keep it this >>> was for simplicity. >> >> I don't like the approach. What we want is: >> - if .bin file exists and is not older, try to load it, >> - if any of the above fails (i.e. .bin file does not exist, is older, or >> cannot be loaded for any reason), then load the text file. >> >> We shouldn't try loading the text file twice. >> >> Also, attached is checkpatch output for your patch. Please fix. > > Also, there is a further wrinkle: Android passes in file_contexts.bin as > the SELABEL_OPT_PATH, so that is the base path. Under the old logic > (before your original clean up patch), we would open that file, detect > that it is binary, and then load it. Under the current logic, we'll > open file_contexts.bin, then try to open file_contexts.bin.bin (which > will fail), and then use the first one. Not true, I don't try to open it, I try to stat it. > > Wondering if we just need to revert. If you want to revert I have no problem with that, but I provided IMO a valid fix. Since I won't likely have a next version patch out till after you go home today, I would support reverting. > >> >>> >>> Signed-off-by: William Roberts <william.c.roberts@intel.com> >>> --- >>> libselinux/src/label_file.c | 41 +++++++++++++++++++++++++++-------------- >>> 1 file changed, 27 insertions(+), 14 deletions(-) >>> >>> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c >>> index 9faecdb..0dee059 100644 >>> --- a/libselinux/src/label_file.c >>> +++ b/libselinux/src/label_file.c >>> @@ -447,7 +447,7 @@ static bool fcontext_is_binary(FILE *fp) >>> #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) >>> >>> static FILE *open_file(const char *path, const char *suffix, >>> - char *save_path, size_t len, struct stat *sb) >>> + char *save_path, size_t len, struct stat *sb, bool force_text) >>> { >>> unsigned int i; >>> int rc; >>> @@ -469,7 +469,11 @@ static FILE *open_file(const char *path, const char *suffix, >>> return NULL; >>> } >>> >>> - for (i = 0; i < ARRAY_SIZE(fdetails); i++) { >>> + size_t array_size = ARRAY_SIZE(fdetails); >>> + if (force_text) >>> + array_size--; >>> + >>> + for (i = 0; i < array_size; i++) { >>> >>> /* This handles the case if suffix is null */ >>> path = rolling_append(stack_path, fdetails[i].suffix, >>> @@ -515,24 +519,33 @@ static int process_file(const char *path, const char *suffix, >>> const char *prefix, struct selabel_digest *digest) >>> { >>> int rc; >>> + unsigned int i; >>> struct stat sb; >>> FILE *fp = NULL; >>> char found_path[PATH_MAX]; >>> >>> - fp = open_file(path, suffix, found_path, sizeof(found_path), &sb); >>> - if (fp == NULL) >>> - return -1; >>> + /* >>> + * first path open the newest modified file, if it fails, the second >>> + * pass falls through to the plain text file. >>> + */ >>> + for(i=0; i < 2; i++) { >>> + fp = open_file(path, suffix, found_path, sizeof(found_path), &sb, >>> + i > 0); >>> + if (fp == NULL) >>> + return -1; >>> >>> - rc = fcontext_is_binary(fp) ? >>> - load_mmap(fp, sb.st_size, rec, found_path) : >>> - process_text_file(fp, prefix, rec, found_path); >>> - if (rc < 0) >>> - goto out; >>> + rc = fcontext_is_binary(fp) ? >>> + load_mmap(fp, sb.st_size, rec, found_path) : >>> + process_text_file(fp, prefix, rec, found_path); >>> + if (!rc) >>> + rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path); >>> >>> - rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path); >>> -out: >>> - fclose(fp); >>> - return rc; >>> + fclose(fp); >>> + >>> + if(!rc) >>> + return 0; >>> + } >>> + return -1; >>> } >>> >>> static void closef(struct selabel_handle *rec); >>> >> >> >> >> _______________________________________________ >> Selinux mailing list >> Selinux@tycho.nsa.gov >> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. >> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. >> > > _______________________________________________ > Seandroid-list mailing list > Seandroid-list@tycho.nsa.gov > To unsubscribe, send email to Seandroid-list-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Seandroid-list-request@tycho.nsa.gov.
On Fri, Sep 16, 2016 at 7:41 AM, William Roberts <bill.c.roberts@gmail.com> wrote: > On Fri, Sep 16, 2016 at 7:38 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >> On 09/16/2016 10:30 AM, Stephen Smalley wrote: >>> On 09/15/2016 07:13 PM, william.c.roberts@intel.com wrote: >>>> From: William Roberts <william.c.roberts@intel.com> >>>> >>>> patch 5e15a52aaa cleans up the process_file() but introduced >>>> a bug. If the binary file cannot be opened, always attempt >>>> to fall back to the textual file, this was not occurring. >>>> >>>> The logic should be: >>>> 1. Open the newest file based on base path + suffix vs >>>> base_path + suffix + ".bin". >>>> 2. If anything fails, attempt base_path + suffix. >>>> >>>> In the case that the file_contexts was the newest file and >>>> used for processing fails, it will attempt the same failure >>>> recovery, which will fail. It was decided to keep it this >>>> was for simplicity. >>> >>> I don't like the approach. What we want is: >>> - if .bin file exists and is not older, try to load it, >>> - if any of the above fails (i.e. .bin file does not exist, is older, or >>> cannot be loaded for any reason), then load the text file. >>> >>> We shouldn't try loading the text file twice. >>> >>> Also, attached is checkpatch output for your patch. Please fix. >> >> Also, there is a further wrinkle: Android passes in file_contexts.bin as >> the SELABEL_OPT_PATH, so that is the base path. Under the old logic >> (before your original clean up patch), we would open that file, detect >> that it is binary, and then load it. Under the current logic, we'll >> open file_contexts.bin, then try to open file_contexts.bin.bin (which >> will fail), and then use the first one. > > Not true, I don't try to open it, I try to stat it. My code never assumes file suffix == type > >> >> Wondering if we just need to revert. > > If you want to revert I have no problem with that, but I provided IMO > a valid fix. > Since I won't likely have a next version patch out till after you go > home today, I > would support reverting. > >> >>> >>>> >>>> Signed-off-by: William Roberts <william.c.roberts@intel.com> >>>> --- >>>> libselinux/src/label_file.c | 41 +++++++++++++++++++++++++++-------------- >>>> 1 file changed, 27 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c >>>> index 9faecdb..0dee059 100644 >>>> --- a/libselinux/src/label_file.c >>>> +++ b/libselinux/src/label_file.c >>>> @@ -447,7 +447,7 @@ static bool fcontext_is_binary(FILE *fp) >>>> #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) >>>> >>>> static FILE *open_file(const char *path, const char *suffix, >>>> - char *save_path, size_t len, struct stat *sb) >>>> + char *save_path, size_t len, struct stat *sb, bool force_text) >>>> { >>>> unsigned int i; >>>> int rc; >>>> @@ -469,7 +469,11 @@ static FILE *open_file(const char *path, const char *suffix, >>>> return NULL; >>>> } >>>> >>>> - for (i = 0; i < ARRAY_SIZE(fdetails); i++) { >>>> + size_t array_size = ARRAY_SIZE(fdetails); >>>> + if (force_text) >>>> + array_size--; >>>> + >>>> + for (i = 0; i < array_size; i++) { >>>> >>>> /* This handles the case if suffix is null */ >>>> path = rolling_append(stack_path, fdetails[i].suffix, >>>> @@ -515,24 +519,33 @@ static int process_file(const char *path, const char *suffix, >>>> const char *prefix, struct selabel_digest *digest) >>>> { >>>> int rc; >>>> + unsigned int i; >>>> struct stat sb; >>>> FILE *fp = NULL; >>>> char found_path[PATH_MAX]; >>>> >>>> - fp = open_file(path, suffix, found_path, sizeof(found_path), &sb); >>>> - if (fp == NULL) >>>> - return -1; >>>> + /* >>>> + * first path open the newest modified file, if it fails, the second >>>> + * pass falls through to the plain text file. >>>> + */ >>>> + for(i=0; i < 2; i++) { >>>> + fp = open_file(path, suffix, found_path, sizeof(found_path), &sb, >>>> + i > 0); >>>> + if (fp == NULL) >>>> + return -1; >>>> >>>> - rc = fcontext_is_binary(fp) ? >>>> - load_mmap(fp, sb.st_size, rec, found_path) : >>>> - process_text_file(fp, prefix, rec, found_path); >>>> - if (rc < 0) >>>> - goto out; >>>> + rc = fcontext_is_binary(fp) ? >>>> + load_mmap(fp, sb.st_size, rec, found_path) : >>>> + process_text_file(fp, prefix, rec, found_path); >>>> + if (!rc) >>>> + rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path); >>>> >>>> - rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path); >>>> -out: >>>> - fclose(fp); >>>> - return rc; >>>> + fclose(fp); >>>> + >>>> + if(!rc) >>>> + return 0; >>>> + } >>>> + return -1; >>>> } >>>> >>>> static void closef(struct selabel_handle *rec); >>>> >>> >>> >>> >>> _______________________________________________ >>> Selinux mailing list >>> Selinux@tycho.nsa.gov >>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. >>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. >>> >> >> _______________________________________________ >> Seandroid-list mailing list >> Seandroid-list@tycho.nsa.gov >> To unsubscribe, send email to Seandroid-list-leave@tycho.nsa.gov. >> To get help, send an email containing "help" to Seandroid-list-request@tycho.nsa.gov. > > > > -- > Respectfully, > > William C Roberts
On 09/16/2016 10:44 AM, William Roberts wrote: > On Fri, Sep 16, 2016 at 7:41 AM, William Roberts > <bill.c.roberts@gmail.com> wrote: >> On Fri, Sep 16, 2016 at 7:38 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >>> On 09/16/2016 10:30 AM, Stephen Smalley wrote: >>>> On 09/15/2016 07:13 PM, william.c.roberts@intel.com wrote: >>>>> From: William Roberts <william.c.roberts@intel.com> >>>>> >>>>> patch 5e15a52aaa cleans up the process_file() but introduced >>>>> a bug. If the binary file cannot be opened, always attempt >>>>> to fall back to the textual file, this was not occurring. >>>>> >>>>> The logic should be: >>>>> 1. Open the newest file based on base path + suffix vs >>>>> base_path + suffix + ".bin". >>>>> 2. If anything fails, attempt base_path + suffix. >>>>> >>>>> In the case that the file_contexts was the newest file and >>>>> used for processing fails, it will attempt the same failure >>>>> recovery, which will fail. It was decided to keep it this >>>>> was for simplicity. >>>> >>>> I don't like the approach. What we want is: >>>> - if .bin file exists and is not older, try to load it, >>>> - if any of the above fails (i.e. .bin file does not exist, is older, or >>>> cannot be loaded for any reason), then load the text file. >>>> >>>> We shouldn't try loading the text file twice. >>>> >>>> Also, attached is checkpatch output for your patch. Please fix. >>> >>> Also, there is a further wrinkle: Android passes in file_contexts.bin as >>> the SELABEL_OPT_PATH, so that is the base path. Under the old logic >>> (before your original clean up patch), we would open that file, detect >>> that it is binary, and then load it. Under the current logic, we'll >>> open file_contexts.bin, then try to open file_contexts.bin.bin (which >>> will fail), and then use the first one. >> >> Not true, I don't try to open it, I try to stat it. > > My code never assumes file suffix == type > >> >>> >>> Wondering if we just need to revert. >> >> If you want to revert I have no problem with that, but I provided IMO >> a valid fix. >> Since I won't likely have a next version patch out till after you go >> home today, I >> would support reverting. Unfortunately it is now entangled with Janis' patch. Let's do this: fix the coding style issues I sent to you from checkpatch, and we'll take this one. Then we'll look to avoid the extraneous load in a subsequent patch.
On Fri, Sep 16, 2016 at 8:00 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 09/16/2016 10:44 AM, William Roberts wrote: >> On Fri, Sep 16, 2016 at 7:41 AM, William Roberts >> <bill.c.roberts@gmail.com> wrote: >>> On Fri, Sep 16, 2016 at 7:38 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >>>> On 09/16/2016 10:30 AM, Stephen Smalley wrote: >>>>> On 09/15/2016 07:13 PM, william.c.roberts@intel.com wrote: >>>>>> From: William Roberts <william.c.roberts@intel.com> >>>>>> >>>>>> patch 5e15a52aaa cleans up the process_file() but introduced >>>>>> a bug. If the binary file cannot be opened, always attempt >>>>>> to fall back to the textual file, this was not occurring. >>>>>> >>>>>> The logic should be: >>>>>> 1. Open the newest file based on base path + suffix vs >>>>>> base_path + suffix + ".bin". >>>>>> 2. If anything fails, attempt base_path + suffix. >>>>>> >>>>>> In the case that the file_contexts was the newest file and >>>>>> used for processing fails, it will attempt the same failure >>>>>> recovery, which will fail. It was decided to keep it this >>>>>> was for simplicity. >>>>> >>>>> I don't like the approach. What we want is: >>>>> - if .bin file exists and is not older, try to load it, >>>>> - if any of the above fails (i.e. .bin file does not exist, is older, or >>>>> cannot be loaded for any reason), then load the text file. >>>>> >>>>> We shouldn't try loading the text file twice. >>>>> >>>>> Also, attached is checkpatch output for your patch. Please fix. >>>> >>>> Also, there is a further wrinkle: Android passes in file_contexts.bin as >>>> the SELABEL_OPT_PATH, so that is the base path. Under the old logic >>>> (before your original clean up patch), we would open that file, detect >>>> that it is binary, and then load it. Under the current logic, we'll >>>> open file_contexts.bin, then try to open file_contexts.bin.bin (which >>>> will fail), and then use the first one. >>> >>> Not true, I don't try to open it, I try to stat it. >> >> My code never assumes file suffix == type >> >>> >>>> >>>> Wondering if we just need to revert. >>> >>> If you want to revert I have no problem with that, but I provided IMO >>> a valid fix. >>> Since I won't likely have a next version patch out till after you go >>> home today, I >>> would support reverting. > > Unfortunately it is now entangled with Janis' patch. Let's do this: fix > the coding style issues I sent to you from checkpatch, and we'll take > this one. Then we'll look to avoid the extraneous load in a subsequent > patch. Fine by me, i'm running to an appointment, I wont have that patch out to probably 3-4pm your time.
On Fri, Sep 16, 2016 at 8:04 AM, William Roberts <bill.c.roberts@gmail.com> wrote: > On Fri, Sep 16, 2016 at 8:00 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >> On 09/16/2016 10:44 AM, William Roberts wrote: >>> On Fri, Sep 16, 2016 at 7:41 AM, William Roberts >>> <bill.c.roberts@gmail.com> wrote: >>>> On Fri, Sep 16, 2016 at 7:38 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >>>>> On 09/16/2016 10:30 AM, Stephen Smalley wrote: >>>>>> On 09/15/2016 07:13 PM, william.c.roberts@intel.com wrote: >>>>>>> From: William Roberts <william.c.roberts@intel.com> >>>>>>> >>>>>>> patch 5e15a52aaa cleans up the process_file() but introduced >>>>>>> a bug. If the binary file cannot be opened, always attempt >>>>>>> to fall back to the textual file, this was not occurring. >>>>>>> >>>>>>> The logic should be: >>>>>>> 1. Open the newest file based on base path + suffix vs >>>>>>> base_path + suffix + ".bin". >>>>>>> 2. If anything fails, attempt base_path + suffix. >>>>>>> >>>>>>> In the case that the file_contexts was the newest file and >>>>>>> used for processing fails, it will attempt the same failure >>>>>>> recovery, which will fail. It was decided to keep it this >>>>>>> was for simplicity. >>>>>> >>>>>> I don't like the approach. What we want is: >>>>>> - if .bin file exists and is not older, try to load it, >>>>>> - if any of the above fails (i.e. .bin file does not exist, is older, or >>>>>> cannot be loaded for any reason), then load the text file. >>>>>> >>>>>> We shouldn't try loading the text file twice. >>>>>> >>>>>> Also, attached is checkpatch output for your patch. Please fix. >>>>> >>>>> Also, there is a further wrinkle: Android passes in file_contexts.bin as >>>>> the SELABEL_OPT_PATH, so that is the base path. Under the old logic >>>>> (before your original clean up patch), we would open that file, detect >>>>> that it is binary, and then load it. Under the current logic, we'll >>>>> open file_contexts.bin, then try to open file_contexts.bin.bin (which >>>>> will fail), and then use the first one. >>>> >>>> Not true, I don't try to open it, I try to stat it. >>> >>> My code never assumes file suffix == type >>> >>>> >>>>> >>>>> Wondering if we just need to revert. >>>> >>>> If you want to revert I have no problem with that, but I provided IMO >>>> a valid fix. >>>> Since I won't likely have a next version patch out till after you go >>>> home today, I >>>> would support reverting. >> >> Unfortunately it is now entangled with Janis' patch. Let's do this: fix >> the coding style issues I sent to you from checkpatch, and we'll take >> this one. Then we'll look to avoid the extraneous load in a subsequent >> patch. > > Fine by me, i'm running to an appointment, I wont have that patch out to > probably 3-4pm your time. BTW did you not get v3 of this patch? I also thought about the additional load attempt even on "textual" files, and I would argue we keep it with a slight modification. The boolean flag passed to open_file should really by called, choose oldest file and invert the time comparison logic. This way, if one file is corrupted, we always attempt to load the other file. Also, all of this code is agnostic to file extension determining content type. This code, with that change would work in the case file_contexts is newer and corrupted, it will try to fall back on binary file. > > -- > Respectfully, > > William C Roberts
On 09/16/2016 01:06 PM, William Roberts wrote: > On Fri, Sep 16, 2016 at 8:04 AM, William Roberts > <bill.c.roberts@gmail.com> wrote: >> On Fri, Sep 16, 2016 at 8:00 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >>> On 09/16/2016 10:44 AM, William Roberts wrote: >>>> On Fri, Sep 16, 2016 at 7:41 AM, William Roberts >>>> <bill.c.roberts@gmail.com> wrote: >>>>> On Fri, Sep 16, 2016 at 7:38 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >>>>>> On 09/16/2016 10:30 AM, Stephen Smalley wrote: >>>>>>> On 09/15/2016 07:13 PM, william.c.roberts@intel.com wrote: >>>>>>>> From: William Roberts <william.c.roberts@intel.com> >>>>>>>> >>>>>>>> patch 5e15a52aaa cleans up the process_file() but introduced >>>>>>>> a bug. If the binary file cannot be opened, always attempt >>>>>>>> to fall back to the textual file, this was not occurring. >>>>>>>> >>>>>>>> The logic should be: >>>>>>>> 1. Open the newest file based on base path + suffix vs >>>>>>>> base_path + suffix + ".bin". >>>>>>>> 2. If anything fails, attempt base_path + suffix. >>>>>>>> >>>>>>>> In the case that the file_contexts was the newest file and >>>>>>>> used for processing fails, it will attempt the same failure >>>>>>>> recovery, which will fail. It was decided to keep it this >>>>>>>> was for simplicity. >>>>>>> >>>>>>> I don't like the approach. What we want is: >>>>>>> - if .bin file exists and is not older, try to load it, >>>>>>> - if any of the above fails (i.e. .bin file does not exist, is older, or >>>>>>> cannot be loaded for any reason), then load the text file. >>>>>>> >>>>>>> We shouldn't try loading the text file twice. >>>>>>> >>>>>>> Also, attached is checkpatch output for your patch. Please fix. >>>>>> >>>>>> Also, there is a further wrinkle: Android passes in file_contexts.bin as >>>>>> the SELABEL_OPT_PATH, so that is the base path. Under the old logic >>>>>> (before your original clean up patch), we would open that file, detect >>>>>> that it is binary, and then load it. Under the current logic, we'll >>>>>> open file_contexts.bin, then try to open file_contexts.bin.bin (which >>>>>> will fail), and then use the first one. >>>>> >>>>> Not true, I don't try to open it, I try to stat it. >>>> >>>> My code never assumes file suffix == type >>>> >>>>> >>>>>> >>>>>> Wondering if we just need to revert. >>>>> >>>>> If you want to revert I have no problem with that, but I provided IMO >>>>> a valid fix. >>>>> Since I won't likely have a next version patch out till after you go >>>>> home today, I >>>>> would support reverting. >>> >>> Unfortunately it is now entangled with Janis' patch. Let's do this: fix >>> the coding style issues I sent to you from checkpatch, and we'll take >>> this one. Then we'll look to avoid the extraneous load in a subsequent >>> patch. >> >> Fine by me, i'm running to an appointment, I wont have that patch out to >> probably 3-4pm your time. > > BTW did you not get v3 of this patch? I did - that's what I ran through checkpatch.pl and sent you the output. > I also thought about the additional load attempt even on "textual" > files, and I would > argue we keep it with a slight modification. The boolean flag passed > to open_file should really > by called, choose oldest file and invert the time comparison logic. > This way, if one file > is corrupted, we always attempt to load the other file. Also, all of > this code is agnostic to > file extension determining content type. This code, with that change > would work in the > case file_contexts is newer and corrupted, it will try to fall back on > binary file. Ok. Just remember that the timestamps might be the same.
diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c index 9faecdb..0dee059 100644 --- a/libselinux/src/label_file.c +++ b/libselinux/src/label_file.c @@ -447,7 +447,7 @@ static bool fcontext_is_binary(FILE *fp) #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) static FILE *open_file(const char *path, const char *suffix, - char *save_path, size_t len, struct stat *sb) + char *save_path, size_t len, struct stat *sb, bool force_text) { unsigned int i; int rc; @@ -469,7 +469,11 @@ static FILE *open_file(const char *path, const char *suffix, return NULL; } - for (i = 0; i < ARRAY_SIZE(fdetails); i++) { + size_t array_size = ARRAY_SIZE(fdetails); + if (force_text) + array_size--; + + for (i = 0; i < array_size; i++) { /* This handles the case if suffix is null */ path = rolling_append(stack_path, fdetails[i].suffix, @@ -515,24 +519,33 @@ static int process_file(const char *path, const char *suffix, const char *prefix, struct selabel_digest *digest) { int rc; + unsigned int i; struct stat sb; FILE *fp = NULL; char found_path[PATH_MAX]; - fp = open_file(path, suffix, found_path, sizeof(found_path), &sb); - if (fp == NULL) - return -1; + /* + * first path open the newest modified file, if it fails, the second + * pass falls through to the plain text file. + */ + for(i=0; i < 2; i++) { + fp = open_file(path, suffix, found_path, sizeof(found_path), &sb, + i > 0); + if (fp == NULL) + return -1; - rc = fcontext_is_binary(fp) ? - load_mmap(fp, sb.st_size, rec, found_path) : - process_text_file(fp, prefix, rec, found_path); - if (rc < 0) - goto out; + rc = fcontext_is_binary(fp) ? + load_mmap(fp, sb.st_size, rec, found_path) : + process_text_file(fp, prefix, rec, found_path); + if (!rc) + rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path); - rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path); -out: - fclose(fp); - return rc; + fclose(fp); + + if(!rc) + return 0; + } + return -1; } static void closef(struct selabel_handle *rec);