On Fri, Sep 16, 2016 at 7:41 AM, William Roberts
<bill.c.robe...@gmail.com> wrote:
> On Fri, Sep 16, 2016 at 7:38 AM, Stephen Smalley <s...@tycho.nsa.gov> wrote:
>> On 09/16/2016 10:30 AM, Stephen Smalley wrote:
>>> On 09/15/2016 07:13 PM, william.c.robe...@intel.com wrote:
>>>> From: William Roberts <william.c.robe...@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.robe...@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
>>> seli...@tycho.nsa.gov
>>> To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
>>> To get help, send an email containing "help" to 
>>> selinux-requ...@tycho.nsa.gov.
>>>
>>
>> _______________________________________________
>> Seandroid-list mailing list
>> Seandroid-list@tycho.nsa.gov
>> To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov.
>> To get help, send an email containing "help" to 
>> seandroid-list-requ...@tycho.nsa.gov.
>
>
>
> --
> Respectfully,
>
> William C Roberts



-- 
Respectfully,

William C Roberts
_______________________________________________
Seandroid-list mailing list
Seandroid-list@tycho.nsa.gov
To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov.
To get help, send an email containing "help" to 
seandroid-list-requ...@tycho.nsa.gov.

Reply via email to