solver: forward nil return value from sharedOp.Exec by jedevc · Pull Request #3043 · moby/buildkit (original) (raw)
Conversation
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
[ Show hidden characters]({{ revealButtonHref }})
Fixes #3040
Exec could sometimes return nil, which would then result in a segmentation fault, as the code attempts to access a field in the nil
result to return. This patch introduces a sanity check before accessing any parts of the field.
| return nil, nil, err |
|---|
| } |
| r := res.(*execRes) |
| if r == nil { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct but is quite a weird construction. Normally typed nil should be avoided and res=nil should be checked. Maybe a comment saying "callback to s.g.Do guarantees that res is always typed *execRes if err or res is nil" would help.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a weird construction. I've reworked the code so that we can just check res==nil.
To do that, I've changed how the method above handles returns - returning s.execRes when s.execRes == nil is not the same as just returning nil, and the new code distinguishes between these two cases.
Exec could sometimes return nil, which would then result in a segmentation fault, as the code attempts to access a field in the nil result to return. This patch introduces a sanity check before accessing any parts of the field.
Signed-off-by: Justin Chadwell me@jedevc.com
| return nil, s.execErr |
|---|
| } |
| if s.execRes != nil { |
| return s.execRes, nil |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this does not atm cache the nil value correctly and Exec gets called again in that case. This can be addressed in follow up.
Is the only case where this can return nil when loading an image without any layers?