Open
Conversation
ony
force-pushed
the
fix/hardlinks-unpack
branch
from
367919e to
a296301
Compare
Owner
|
This should be fixed by a small rewrite now pushed to master. In particular, tests now pass. Thanks for getting to the bottom of this! |
Contributor
Author
|
@vmchale, thank you for considering this PR. I actually have concerns about code like this: preHardlink <- liftIO $ archiveEntryHardlink x -- pointer to current hardlink
hardlink <- liftIO $ peekCString preHardlink
let hardlink' = fp </> hardlink
liftIO $ withCString hardlink' $ \hl ->
archiveEntrySetHardlink x hl -- not sure if preHardLink is still valid after this
ignore $ archiveReadExtract a x archiveExtractTime
liftIO $ archiveEntrySetPathname x preFile
liftIO $ archiveEntrySetHardlink x preHardlinkI thought with a296301 I made this code a bit safer and hack itself more isolated. |
Contributor
Author
|
Valgrind proof for d2b34ac : And for 97f80b1 : Please re-consider. Note that restoring entries might not be very useful. We are not re-using them, I guess. And if we would then we need to ensure that it doesn't happen in separate threads. Ehhh.. Unfortunately Haskell doesn't have destructible input like in Mercury language. |
This also fixes access to freed memory. See comments in vmchale#4
ony
force-pushed
the
fix/hardlinks-unpack
branch
from
a296301 to
c785a0b
Compare
Contributor
Author
vmchale
reviewed
Dec 5, 2019
| -- See https://github.com/libarchive/libarchive/issues/1203 | ||
| withPrefix :: FilePath -> ArchiveEntryPtr -> IO a -> IO a | ||
| withPrefix fp x inner = do | ||
| file0 <- peekCString =<< archiveEntryPathname x |
Owner
There was a problem hiding this comment.
Doesn't this mean the pointer to the original archive entry pathname is forgotten? Thus, the original pathname will never be freed.
Contributor
Author
There was a problem hiding this comment.
Declaration is const char * archive_entry_pathname(struct archive_entry *a). Thus ownership for returned pointer is not transferred to caller as in char *strdup(const char*). It still owned by archive_entry structure.
You can find similar code in readEntry:
Entry
<$> (peekCString =<< archiveEntryPathname entry)
<*> readContents a entry
-- ...
vmchale
requested changes
Dec 5, 2019
Owner
There was a problem hiding this comment.
The test suite fails with
Test suite libarchive-test: RUNNING...
roundtrip
sucessfully unpacks/packs (test/data/libarchive-1.0.5.1.tar.gz) FAILED [1]
Failures:
test/Spec.hs:26:23:
1) roundtrip sucessfully unpacks/packs (test/data/libarchive-1.0.5.1.tar.gz)
predicate failed on: Left ArchiveFatal
To rerun use: --match "/roundtrip/sucessfully unpacks/packs (test/data/libarchive-1.0.5.1.tar.gz)/"
Randomized with seed 1103473752
Finished in 0.0004 seconds
1 example, 1 failure
on my computer.
You need to run make -j so that the full test suite runs :)
Contributor
Author
|
Contributor
Author
|
@vmchale , I suspect you have corrupted tar files. Please check that output of: On c785a0b error doesn't appear after executing: P.S. For some reason my |
Contributor
Author
|
Heh. Found it:
Culprit is |
Owner
vmchale
approved these changes
Dec 6, 2019
Contributor
Author
|
@vmchale, thank you for fixing this issue. I confirm that now there is no reports of invalid memory access on my computer. I believe it is fine to close this PR. I've noticed that you introduced CI. That's a very useful for these situations when people are having different results. Since this library interacts with C directly it might be useful to add valgrind check. After checking out github search, I think diff --git a/.github/workflows/haskell.yml b/.github/workflows/haskell.yml
index 69982f2..cfb5683 100644
--- a/.github/workflows/haskell.yml
+++ b/.github/workflows/haskell.yml
@@ -7,6 +7,8 @@ jobs:
with:
cabal-version: "3.0"
ghc-version: "8.8.1"
+ - name: "Install valgrind"
+ run: "sudo apt-get install -y valgrind"
- name: "Install libarchive"
run: |
wget https://www.libarchive.org/downloads/libarchive-3.4.0.tar.gz
@@ -27,8 +29,11 @@ jobs:
make -j
- name: Tests
run: "cabal test"
+ - name: "Valgrind tests"
+ run: "valgrind dist/build/libarchive-test/libarchive-test"
- name: Documentation
run: "cabal haddock"
name: "Haskell CI"
on:
- push
+ - pull_requestIt seems that |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes #4 (at least those that are exposed in test).
We should monitor libarchive/libarchive#1203 for a better solution than temporary amending archive entries.
P.S. Commit is done on top PR #2 to show results.