On Fri, Sep 16, 2016 at 7:38 AM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> On 09/16/2016 10:30 AM, Stephen Smalley 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.
>>
>> 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@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);
>>>
>>
>>
>>
>> _______________________________________________
>> 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.
>>
>
> _______________________________________________
> 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.
