-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (FWIW :) )
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. |
No problem. :) I did read it tho! :) |
} | ||
|
||
imageFilePath := filepath.Join(params.OutputDir, "image") | ||
imageFile, err := os.Create(imageFilePath) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 callsManager.createTestConfig()
withimageDir
=tmpDir
(which corresponds to theoutputDir
in this build script at the time of the initial build). Manager.createTestConfig()
partially populates amgrconfig.Config
. (It does not populate the Image field.) Then,Manager.createTestConfig()
callsinstance.SetConfigImage()
with the partially-populated struct and theimageDir
string.instance.SetConfigImage()
sets themgrconfig.Config
'sImage
field to "${tmpDir}/image", so that theImage
field is no longer empty. The slightly-more-populated config struct is returned back tocreateTestConfig()
.- Later,
createTestConfig()
callsmgrconfig.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.
@@ -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") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
Line 800 in 89298aa
kernelObjFile := filepath.Join(assetFolder, "obj", target.KernelObject) |
Also it's used for symbolization of reports, so may be useful even w/op coverage.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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.
After this, syz-ci can build Starnix and run syz-manager with a manager config with
"type": "starnix"
.