On Mon, Nov 13, 2006 at 04:36:01PM +0100, Eric Y. Kow wrote:
> I'm going to accept these patches for more testing. If I understand
> correctly, the changes are that:
>
> 1. the check is performed in the outer layer (commands)
> instead of within the inner layer (patches)
> 2. the check is now optional (and it is the possibility of disabling this
> check
> that resolves issue177
Correct.
> > The main problem with the old way is the apply function in
> > PatchApply is not the only "entry point" to applying patches.
>
> It's not? That's interesting. What other entry points are there?
> [ i'm not asking for a full list :-) ]
Here are the main points of the problem:
* apply calls apply_list for composite patches, which recursively
calls apply for the sub patches, unless sometimes it performs
the apply itself, for example to handle the
set-script-executable hack. (This is how I discovered there were
problems.)
* force_replace_slurpy is an exported function that doesn't call
apply.
* apply_to_filepaths and apply_to_slurpy are exported functions
(that does call apply, I think, but) that removes the options
(e.g., to turn the check off) for some particular reason.
* Git imports applyBinary and applyHunkLines (very primitive
functions) and uses them directly.
All of this can be handled (Turing complete, and so on...), but
there are other issues too. Darcs often apply the same patch
more than once, e.g., to both working and pristine, and also to
produce intermittent results. Checking the same patch over and
over again is not good. It's better to check on every write or
every read. Checking writes is probably safest. Checking reads
gives the best error messages. Which of checking reads or
checking writes is most efficient is harder to tell. It depends
on if darcs reads many more patches without writing them then
writing patches several times.
> > It unfortunately only checks
> > remote patches fetched with Apply or Pull, but it does check
> > them reliably.
>
> Did you overlook Pull by any chance? I see only code for checking with
> Check and Apply. Note that Pull does not invoke anything from Apply,
> so either a refactor or a separate check would be necessary.
Argh! I removed the wrong lines. The patch contains the Check
check that freezes darcs without feedback for a while. I'll
attach a patch that restores order.
> > +is_malicious_path :: String -> Bool
> > +is_malicious_path fp =
> > + not $ is_explicitly_relative fp &&
> > + null (intersect (breakup fp) [ "..", "_darcs" ])
>
> I personally found the old code easier to understand. How about
> something like this
>
> is_malicious_path :: FilePath -> Bool
> is_malicious_path fp =
> not (is_explicitly_relative fp) ||
> breakup fp `contains_any` [ "..", "_darcs" ]
> where
> contains_any a b = not . null $ intersect a b
I'll throw this in with an extra patch too.
--
Tommy Pettersson <[EMAIL PROTECTED]>
New patches:
[really use new malicious file path check in pull (not in changes)
Tommy Pettersson <[EMAIL PROTECTED]>**20061113182628]
<
> {
hunk ./Check.lhs 27
import List ( sort )
import DarcsCommands ( DarcsCommand(..), nodefaults )
-import DarcsCommandsAux ( check_paths )
import DarcsArguments ( DarcsFlag( Quiet, Verbose, NoTest, LeaveTestDir ),
partial_check, any_verbosity, notest,
leave_test_dir, working_repo_dir,
hunk ./Check.lhs 112
case patch2patchinfo chk of
Just chtg -> do
putVerbose $ text "I am checking from a checkpoint."
- let ps = (chtg, Just chk) :
- reverse (concat $ get_patches_beyond_tag chtg patches)
- check_paths opts $ map (fromJust.snd) ps
- apply_patches_with_feedback [] False feedback putInfo ps
+ apply_patches_with_feedback [] False feedback putInfo
+ $ (chtg, Just chk)
+ : reverse (concat $ get_patches_beyond_tag chtg patches)
Nothing -> impossible
hunk ./Check.lhs 116
- Nothing -> do check_paths opts $ map (fromJust.snd) (concat patches)
- apply_patches_with_feedback [] False feedback putInfo $
- reverse $ concat patches
+ Nothing -> apply_patches_with_feedback [] False feedback putInfo $
+ reverse $ concat patches
is_same <-
withCurrentDirectory cwd $ (identifyPristine >>= checkPristine chd)
if is_same
hunk ./Pull.lhs 27
import SignalHandler ( withSignalsBlocked )
import DarcsCommands ( DarcsCommand(..), loggers )
+import DarcsCommandsAux ( check_paths )
import DarcsArguments ( DarcsFlag( AnyOrder, Test, Verbose, Quiet,
Intersection ),
want_external_merge, nocompress, ignoretimes,
hunk ./Pull.lhs 149
when (null to_be_pulled) $ do
logMessage "You don't want to pull any patches, and that's fine with me!"
exitWith ExitSuccess
+ check_paths opts to_be_pulled
putVerbose $ text "Getting and merging the following patches:"
putVerbose $ format_patches_inventory to_be_pulled
(pc,pw) <- merge_with_us_and_pending opts
}
[refactor is_malicious_path (for easier reading)
Tommy Pettersson <[EMAIL PROTECTED]>**20061113191040]
<
> hunk ./DarcsCommandsAux.lhs 92
is_malicious_path :: String -> Bool
is_malicious_path fp =
- not $ is_explicitly_relative fp &&
- null (intersect (breakup fp) [ "..", "_darcs" ])
+ not (is_explicitly_relative fp) ||
+ breakup fp `contains_any` [ "..", "_darcs" ]
+ where
+ contains_any a b = not . null $ intersect a b
\end{code}
Context:
[remove old malicious_filename check (issue177)
Tommy Pettersson <[EMAIL PROTECTED]>**20061110211757]
[use new malicious file path check in pull and apply (issue177)
Tommy Pettersson <[EMAIL PROTECTED]>**20061110211702]
[fix latex markup error
Tommy Pettersson <[EMAIL PROTECTED]>**20061110205511]
[add new malicious file path check system
Tommy Pettersson <[EMAIL PROTECTED]>**20061110132338
Adds a new module DarcsCommandsAux for auxiliary functionality common to
more than one darcs command.
]
[add function for finding all file names in a patch
Tommy Pettersson <[EMAIL PROTECTED]>**20061109144144]
[Look for Text.Regex in package regex-compat. Needed for GHC 6.6
Josef Svenningsson <[EMAIL PROTECTED]>**20061004123158]
[bumb version to 1.0.9rc2
Tommy Pettersson <[EMAIL PROTECTED]>**20061009204226]
[TAG 1.0.9rc1
Tommy Pettersson <[EMAIL PROTECTED]>**20061008175207]
Patch bundle hash:
0be991fec3e03bee40329af8bc9830f786e2fdb3
_______________________________________________
darcs-devel mailing list
[email protected]
http://www.abridgegame.org/cgi-bin/mailman/listinfo/darcs-devel
