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

fix empty arguments resolution #4442

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ShohamBit
Copy link
Collaborator

@ShohamBit ShohamBit commented Dec 18, 2024

1. Explain what the PR does

"Replace me with make check-pr output"

2. Explain how to test it

change empty sting to hex value

3. Other comments

Resolves #3892

@yanivagman
Copy link
Collaborator

Let's change all parsers inside this file that currently return an empty value on an error to return the raw value instead

@ShohamBit
Copy link
Collaborator Author

Let's change all parsers inside this file that currently return an empty value on an error to return the raw value instead.

Okay, but I think the code is still going to look weird because if we just print the information without any need for processing it behind the scenes, why would we write the function like this:

func funcArg(arg *trace.Argument, val uint64) {
    arg.Type = "string"
    argument, err := parsers.funcArg(mode) 
    if err != nil {
        arg.Value = "" 
        return 
    }
    arg.Value = argument.String() 
}

instead of like this:

func funcArg(arg *tracee.Argument, val uint64) {
    arg.Type = "string"
    arg.Value = val 
}

or just remove it entirely?

It looks pretty weird that we check for an error and don't even use it.

Note: Not every function behaves like this, but most of them do.

@oshaked1
Copy link
Contributor

It looks pretty weird that we check for an error and don't even use it.

We do use the error, that way we know to use the raw value.

@@ -46,7 +46,7 @@ func parseBPFProgType(arg *trace.Argument, progType uint64) {
arg.Type = "string"
bpfProgTypeArgument, err := parsers.ParseBPFProgType(progType)
if err != nil {
arg.Value = ""
arg.Value = progType
Copy link
Contributor

@oshaked1 oshaked1 Dec 22, 2024

Choose a reason for hiding this comment

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

If the raw value is used, the value type is inconsistent. We should either change the type accordingly or format the raw value as a string (preferrable in my opinion).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree - better to format the raw argument as a string

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that sounds good. However, as discussed earlier, raw values are used when Tracee can't identify the data. In this code, forming the raw value isn't the best practice because that happens in the pkg/parsers module.

@@ -157,7 +157,7 @@ func parseBPFCmd(arg *trace.Argument, cmd uint64) {
arg.Type = "string"
bpfCommandArgument, err := parsers.ParseBPFCmd(cmd)
if err != nil {
arg.Value = ""
arg.Value = cmd
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@yanivagman
Copy link
Collaborator

Let's change all parsers inside this file that currently return an empty value on an error to return the raw value instead.

Okay, but I think the code is still going to look weird because if we just print the information without any need for processing it behind the scenes, why would we write the function like this:

func funcArg(arg *trace.Argument, val uint64) {
    arg.Type = "string"
    argument, err := parsers.funcArg(mode) 
    if err != nil {
        arg.Value = "" 
        return 
    }
    arg.Value = argument.String() 
}

instead of like this:

func funcArg(arg *tracee.Argument, val uint64) {
    arg.Type = "string"
    arg.Value = val 
}

or just remove it entirely?

It looks pretty weird that we check for an error and don't even use it.

Note: Not every function behaves like this, but most of them do.

Because the parsers return the parsed value of the argument. For example, if some (fake) argument has a raw value 0x4 which means F_WRITE, the output string will be "F_WRITE". However, if it has a value of 0x20, but the parsers don't know this value, an error will be returned. In this case, we can simply return the string value of the raw argument, which is "0x20"

@ShohamBit ShohamBit changed the title fix issue 3892 fix empty arguments resolution Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure in argument name resolution results in empty value
3 participants