[go: up one dir, main page]

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

pkg/build: add build command for starnix #5327

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

glpesk
Copy link
Collaborator
@glpesk glpesk commented Sep 19, 2024

After this, syz-ci can build Starnix and run syz-manager with a manager config with "type": "starnix".

Copy link
Collaborator
@noncombatant noncombatant left a comment

Choose a reason for hiding this comment

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

LGTM (FWIW :) )

@glpesk glpesk removed the request for review from mvanotti September 19, 2024 23:57
@glpesk
Copy link
Collaborator Author
glpesk commented Sep 20, 2024

Hi!! Sorry about that, you still show up as an auto-assigned reviewer even though you're emeritus, and I thought I'd managed to take you off the notification list. I think we'll need help from the syzkaller admins to change the default list of reviewers.

@noncombatant
Copy link
Collaborator

No problem. :) I did read it tho! :)

}

imageFilePath := filepath.Join(params.OutputDir, "image")
imageFile, err := os.Create(imageFilePath)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to create an empty file here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The short answer is that syz-ci's image test checks that this file exists, and the image test fails if not. The contents of the file are not used by the test, so an empty file here is enough to get syz-ci working.

Sorry that I didn't look for a better solution here yet. I think there might be an unintended interaction between two methods involved in the image test, leading to an unnecessary(?) check for this file.

The sequence is:

  • syz-ci's Manager.build() indirectly calls Manager.createTestConfig() with imageDir = tmpDir (which corresponds to the outputDir in this build script at the time of the initial build).
  • Manager.createTestConfig() partially populates a mgrconfig.Config. (It does not populate the Image field.) Then, Manager.createTestConfig() calls instance.SetConfigImage() with the partially-populated struct and the imageDir string.
  • instance.SetConfigImage() sets the mgrconfig.Config's Image field to "${tmpDir}/image", so that the Image field is no longer empty. The slightly-more-populated config struct is returned back to createTestConfig().
  • Later, createTestConfig() calls mgrconfig.Complete() on the partially-populated config struct. That method does the following check:
if cfg.Image != "" {
		if !osutil.IsExist(cfg.Image) {
			return fmt.Errorf("bad config param image: can't find %v", cfg.Image)
		}
		cfg.Image = osutil.Abs(cfg.Image)
}

Since the config's Image field is not empty if it has come from createTestConfig(), we hit the existence check in that case.

My guess is that the right solution is to create the image path in instance.SetConfigImage() and check whether that file exists, and proceed without setting the config's Image field if not.

pkg/build/starnix.go Show resolved Hide resolved
@@ -171,7 +171,9 @@ func (env *env) BuildKernel(buildCfg *BuildKernelConfig) (
}

func SetConfigImage(cfg *mgrconfig.Config, imageDir string, reliable bool) error {
cfg.KernelObj = filepath.Join(imageDir, "obj")
if cfg.Type != targets.Starnix {
cfg.KernelObj = filepath.Join(imageDir, "obj")
Copy link
Collaborator

Choose a reason for hiding this comment

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

It will probably take significant work to make it work correctly, but it seems like we might eventually want to have the starnix binary available for doing e.g. coverage reports... does creating an empty file (or copying over the starnix_runner binary as-is) cause issues currently?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it would be better to create an empty file in "obj/", or copy real binary there.
syz-ci also knows about this layout and will upload the object file and provide in bug reports:

kernelObjFile := filepath.Join(assetFolder, "obj", target.KernelObject)

Also it's used for symbolization of reports, so may be useful even w/op coverage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, this might be possible to do now -- I'm not 100% sure which file is the right one to use as the kernel obj here, but there are only a few options. I'll try to get more info and update soon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is out/x64/exe.unstripped/starnix_kernel

Copy link
Collaborator
@dvyukov dvyukov left a comment

Choose a reason for hiding this comment

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

But otherwise looks good to me.

@glpesk glpesk marked this pull request as draft September 24, 2024 22:45
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.

4 participants