fix: chalking large zip files #229

miki725 · 2024-03-01T20:01:28Z

Followed the steps in the contributor's guide: https://crashoverride.com/docs/other/contributing#filing-the-pull-request
PR title uses semantic commit messages
Filled out the template to a useful degree

Issue

none of the codecs were using chalk fd caching machinery and some of them were leaking FDs which was causing issues when trying to chalk a jar file with ~3K files

fixes #225
fixes #230

Description

ensure consistent use of the FD cache mechanism across chalk codebase. previous each ChalkObj contained its own stream: FileStream attribute which was used for caching that paths FD. that had some issues such as it was impossible to use that mechanism outside of ChalkObj such as in codecs as they are responsible for create ChalkObj in the first place. to make it all consistent:

create generic fd_cache implementation independent of anything chalk-specific (eventually to be moved into nimutils)
refactor codecs to use generic fd_cache implementation
remove stream from ChalkObj and instead use the fd_cache from above
refactor existing chalk cache to use the fd_cache

Testing

existing tests plus some tests on large zip files such as zap jar file which has ~3K files

Removing previous chalk FD caching mechanism which stored file stream on the chalk object which restricted where it can be used. Now most places use the fd_cache mechanism to interact with FDs except a few specific places which need to do something custom such as open FD to TTY which is not cached.

If we open by default with fmReadWriteExisting mode during all codec chalk scans, it leaves all FDs open in write mode which means any other program using flock on the same file will not work. For example we scan docker binary to determine if its chalked and docker seems to use flock and so as chalk would open it in write mode docker would not be executable in a subprocess. By explicitly specifying the file mode it allows the caller to determine how to open the file hence avoiding these type of conflicts, which are especially pronounced when multiple chalks run in parallel on the same machine.

otherwise for example running lots of plugins on non-valid files such as docker image name was not working as expected as there is no corresponding file

otherwise any subscan triggered within chalk command would reset context directories and therefore plugin collection was not working as expected this was impacting zip codec so far

viega

I think this is very well done. The only thing is that 'yield' no longer has a correct meaning, so I think to avoid future confusion, please do change the naming as I mention in other comments!~

viega · 2024-03-05T18:22:32Z

src/commands/cmd_docker.nim

-  trace("New docker file: \n" & newcontents)
-  f.write(newcontents)
-  f.close()
+  let (f, path) = getNewTempFile()


This should have an analogue writeNewTempFile() that does the stuff you do below.

good idea. added in crashappsec/nimutils@c523586

viega · 2024-03-05T18:24:32Z

src/docker_git.nim

@@ -54,8 +54,10 @@ proc createTempKnownHosts(data: string): string =
  if data == "":
    return ""
  let (f, path) = getNewTempFile()
-  f.write(data)
-  f.close()
+  try:


See above, should have a 'writeNewTempFile()`; Plus, it occurs to me we should have a semgrep rule to warn on new adds of getNewTempFile()

cc @indecisivedragon was just mentioning semgrep to me as well. good idea to add that to chalk CI

viega · 2024-03-05T18:26:06Z

src/fd_cache.nim

+## for all opened file streams.
+##
+## Some things you can do:
+## * yield   - create or get existing file stream from cache.


'yield' generally means "I'm done with this (for now)". Generally, I would take that to mean the system can decide whether it needs to be closed (whereas close generally asserts we need to close). 'Acquire' is the right word to use here.

was going between the 2 but yeah the logic for acquire makes more sense

renamed in 411b868 (#229)

viega · 2024-03-05T18:27:49Z

src/fd_cache.nim

+
+# ----------------------------------------------------------------------------
+
+proc getOpenLimit(): int =


It may be worth limiting ourselves to some percentage of the rlimit (or subtract out a healthy number). Hard to know what 3rd party stuff we'll pull in that can't use the API.

yep. we already do. we still honor the cache_fd_limit config:

chalk/src/util.nim

Line 301 in caf6e27

limitFDCacheSize(chalkConfig.getCacheFdLimit())

and it checks that value is less than 1/2 of the rlimit:

chalk/src/fd_cache.nim

Lines 223 to 234 in caf6e27

let

# dont use all FDs in the cache and allow other descriptors

# to be opened in external libs/etc

fdLimit = getOpenLimit() div 2

fdCache = newFDCache(size = fdLimit)

proc limitFDCacheSize*(size: int) =

if size > fdLimit:

raise newException(OSError,

"attempting to set FD cache size limit to " & $size &

" which is too large given system limit of " & $fdLimit)

fdCache.limitSize(size)

also for context default fd limit in the config is 50:

chalk/src/configs/chalk.c42spec

Lines 2524 to 2527 in 938e474

field cache_fd_limit {

type: int

default: 50

range: (0, high())

so we should be ok for most systems. dont imagine well see anything much lower with rlimit of 1024

viega · 2024-03-05T18:30:16Z

src/plugins/codecMacOs.nim

-          "Replace the script or rename the executable")
-        scanFail()
-      # Drop down below for the chalk mark.
+  # chalked mac binary is a macho binary wrapped as shell script


Mach-O is not macho 🤣

viega · 2024-03-05T18:31:57Z

src/plugins/codecZip.nim


-  return some(chalk)
+    let


Someday we should really use an in-memory option here :/ Not now of course.

also using writeNewTempFile from nimtuils for more consise temp file handling in chalk

refactor: adding fd_cache and using it in plugins

b614894

miki725 requested review from indecisivedragon and ee7 March 1, 2024 20:01

miki725 force-pushed the ms/fd branch from 10ccb32 to 52103e4 Compare March 5, 2024 00:17

miki725 changed the title ~~WIP refactor: adding fd_cache and using it in plugins~~ WIP: fix fd leak by using gloabl fd cache Mar 5, 2024

miki725 force-pushed the ms/fd branch from eb39d0d to 6ba994f Compare March 5, 2024 03:00

fix: using non-strict mode in all codecs for file interactions

56804aa

otherwise for example running lots of plugins on non-valid files such as docker image name was not working as expected as there is no corresponding file

miki725 changed the title ~~WIP: fix fd leak by using gloabl fd cache~~ fix fd leak by using gloabl fd cache Mar 5, 2024

miki725 changed the title ~~fix fd leak by using gloabl fd cache~~ fix: fd leak by using gloabl fd cache Mar 5, 2024

miki725 marked this pull request as ready for review March 5, 2024 04:35

miki725 requested a review from viega as a code owner March 5, 2024 04:35

miki725 added 2 commits March 5, 2024 10:12

fix: subscan honors context directories

135c3ac

otherwise any subscan triggered within chalk command would reset context directories and therefore plugin collection was not working as expected this was impacting zip codec so far

docs: updating CHANGELOG about fd leak + git keys

caf6e27

miki725 changed the title ~~fix: fd leak by using gloabl fd cache~~ fix: chalking large zip files Mar 5, 2024

viega requested changes Mar 5, 2024

View reviewed changes

miki725 force-pushed the ms/fd branch from 5bfadee to 411b868 Compare March 5, 2024 19:39

refactor: renaming yieldFileStream to acquireFileStream

938e474

also using writeNewTempFile from nimtuils for more consise temp file handling in chalk

miki725 force-pushed the ms/fd branch from 411b868 to 938e474 Compare March 5, 2024 19:42

miki725 requested a review from viega March 5, 2024 19:46

viega approved these changes Mar 6, 2024

View reviewed changes

miki725 merged commit eb36230 into main Mar 6, 2024
2 checks passed

miki725 deleted the ms/fd branch March 6, 2024 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: chalking large zip files #229

fix: chalking large zip files #229

miki725 commented Mar 1, 2024 •

edited

Loading

viega left a comment

viega Mar 5, 2024

miki725 Mar 5, 2024

viega Mar 5, 2024

miki725 Mar 5, 2024

viega Mar 5, 2024

miki725 Mar 5, 2024

miki725 Mar 5, 2024 •

edited

Loading

viega Mar 5, 2024

miki725 Mar 5, 2024

miki725 Mar 5, 2024

viega Mar 5, 2024

miki725 Mar 5, 2024

viega Mar 5, 2024


		# ----------------------------------------------------------------------------

		proc getOpenLimit(): int =

	let
	# dont use all FDs in the cache and allow other descriptors
	# to be opened in external libs/etc
	fdLimit = getOpenLimit() div 2
	fdCache = newFDCache(size = fdLimit)

	proc limitFDCacheSize*(size: int) =
	if size > fdLimit:
	raise newException(OSError,
	"attempting to set FD cache size limit to " & $size &
	" which is too large given system limit of " & $fdLimit)
	fdCache.limitSize(size)

	field cache_fd_limit {
	type: int
	default: 50
	range: (0, high())

fix: chalking large zip files #229

fix: chalking large zip files #229

Conversation

miki725 commented Mar 1, 2024 • edited Loading

Issue

Description

Testing

viega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miki725 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miki725 commented Mar 1, 2024 •

edited

Loading

miki725 Mar 5, 2024 •

edited

Loading