On 09/16/2016 01:06 PM, William Roberts wrote: > On Fri, Sep 16, 2016 at 8:04 AM, William Roberts > <bill.c.roberts@xxxxxxxxx> wrote: >> On Fri, Sep 16, 2016 at 8:00 AM, Stephen Smalley <sds@xxxxxxxxxxxxx> 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@xxxxxxxxx> wrote: >>>>> 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. >>>> >>>> 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. _______________________________________________ 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.
