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.
>
> 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);
>
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.
_______________________________________________
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.
