Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CP-49213: Add new tar unpacking module #5787

Conversation

BengangY
Copy link
Contributor

@BengangY BengangY commented Jul 8, 2024

Add a module Tar_ext to unpack a tar file.
During the unpacking process, verify the tar file, containing total file size,
file type, file path, file integrity, etc.

@BengangY BengangY changed the base branch from master to feature/non-cdn-update July 8, 2024 01:41
@BengangY BengangY marked this pull request as ready for review July 8, 2024 01:57
Copy link
Contributor

@edwintorok edwintorok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add the script that generated the tar file instead of the binary tar files?
Binary files are best avoided, even if just for test code.

ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/xapi_globs.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.mli Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
@psafont
Copy link
Member

psafont commented Jul 8, 2024

Can this module be used to replace existing tar-unpacking code?

Copy link
Contributor

@Vincent-lau Vincent-lau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is xapi/ the right place for this module? I feel this is just a utility module rather than a xapi functionality?

ocaml/xapi/tar_ext.ml Show resolved Hide resolved
ocaml/xapi/tar_ext.mli Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
@BengangY
Copy link
Contributor Author

BengangY commented Jul 8, 2024

Can this module be used to replace existing tar-unpacking code?

Some code can be replaced. But most of the tar-unpacking code doesn't unpack the files to disk. The extracted files are processed directly and not saved on disk.

@gangj
Copy link
Contributor

gangj commented Jul 9, 2024

Can this module be used to replace existing tar-unpacking code?

Some code can be replaced. But most of the tar-unpacking code doesn't unpack the files to disk. The extracted files are processed directly and not saved on disk.

Can we make "saving to files" or "extract into some memory" options of this module?

@BengangY
Copy link
Contributor Author

BengangY commented Jul 9, 2024

Can this module be used to replace existing tar-unpacking code?

Some code can be replaced. But most of the tar-unpacking code doesn't unpack the files to disk. The extracted files are processed directly and not saved on disk.

Can we make "saving to files" or "extract into some memory" options of this module?

I don't think we need this option. It can call directly Tar_unix.Archive.with_next_file to process tar data in memory by self-defined function.

ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
@lindig
Copy link
Contributor

lindig commented Jul 11, 2024

Files changed 302 - something is wrong here. You can't expect us to review this.

@BengangY
Copy link
Contributor Author

Files changed 302 - something is wrong here. You can't expect us to review this.

Yes, I was trying to resolve the build and test failure. Now it's fixed.

ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/tests/test_tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
ocaml/xapi/tar_ext.mli Outdated Show resolved Hide resolved
@BengangY BengangY force-pushed the private/bengangy/CP-49213 branch 2 times, most recently from 13c0b05 to 548b1f9 Compare July 15, 2024 08:39
ocaml/xapi/tar_ext.ml Outdated Show resolved Hide resolved
Comment on lines +25 to +34
let create_temp_dir () =
let mktemp = Cmd.v "mktemp" in
let mktemp' = Cmd.(mktemp % "-d") in
let result = OS.Cmd.(run_out mktemp' |> to_string) in
match result with
| Ok path ->
path
| Error (`Msg s) ->
Alcotest.fail
(Printf.sprintf "Test tar_ext creating temp dir failure: %s" s)
Copy link
Contributor

@gangj gangj Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic should be put in gen_tar_ext_test_file.sh as otherwise it will be required every where the gen_tar_ext_test_file.sh is called.

@lindig
Copy link
Contributor

lindig commented Jul 16, 2024

I would like to suggest to think about the following approach: use Unix.realpath to resolve a path coming from a Tar file and see that it is prefixed by the current directory and reject it otherwise.

@gangj
Copy link
Contributor

gangj commented Jul 17, 2024

Just a question for this tar unpacking feature, what's the performance impact (CPU usage) on the host as the unpacking might take long time for a big file, I think we should avoid consuming too much CPU of the host.

ocaml/xapi/tar_ext.mli Outdated Show resolved Hide resolved
Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I now understand the protection against escaping the directory better and think it's ok (checking against paths ., .., and /).

@BengangY
Copy link
Contributor Author

Just a question for this tar unpacking feature, what's the performance impact (CPU usage) of the host as the unpacking might take long time for a big file, I think we should avoid consuming too much CPU of the host.

just tested it, I uploaded a tar file of about 500M, and it used not more than 8% (1 processor of 16). This took about 1 second as I uploaded the file from local, so it finished soon.

Add a module Tar_ext to unpack a tar file.
During the unpacking process, verify the tar file, containing total file size,
file type, file path, file integrity, etc.

Signed-off-by: Bengang Yuan <bengang.yuan@cloud.com>
Signed-off-by: Bengang Yuan <bengang.yuan@cloud.com>
@minglumlu minglumlu merged commit bfa96cb into xapi-project:feature/non-cdn-update Jul 17, 2024
15 checks passed
@BengangY
Copy link
Contributor Author

Is xapi/ the right place for this module? I feel this is just a utility module rather than a xapi functionality?

I don't find a proper place for it. But it's only used for xapi in the current stage, so I think it's OK to place here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants