Re: [PATCH v3] libselinux: correct error path to always try text

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Sep 16, 2016 at 7:30 AM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> On 09/15/2016 07:13 PM, william.c.roberts@xxxxxxxxx wrote:
>> From: William Roberts <william.c.roberts@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxx
> To unsubscribe, send email to Seandroid-list-leave@xxxxxxxxxxxxx.
> To get help, send an email containing "help" to Seandroid-list-request@xxxxxxxxxxxxx.



-- 
Respectfully,

William C Roberts
_______________________________________________
Selinux mailing list
Selinux@xxxxxxxxxxxxx
To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.



[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux