[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.

Sign up for GitHub

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

Jump to bottom

fix: avoid printing confusing message when input contains special character #495

Merged
merged 5 commits into from
Feb 29, 2024
Merged

fix: avoid printing confusing message when input contains special character #495

merged 5 commits into from
Feb 29, 2024

Conversation

ruitianzhong
Copy link
Contributor

The following usage would printed out confusing message when user controlled input contains special character like "%s".

Query field is controlled by user and may contains special character.

s := fmt.Sprintf(fmt.Sprintf(" PID: %d, Comm: %s, Time: %d, Query: %s", pe.Pid, pe.Comm, pe.Timestamp, unix.ByteSliceToString((pe.Query[:]))))

Reproduce

run the following command in the terminal:

sudo ./ecapture bash

run the following command in another terminal:

 echo "SELECT * FROM Customers WHERE CustomerName LIKE 'a%'"

output:

bash_2024/02/29 11:26:21 PID:33569, UID:1000, 	Comm:bash, 	Retvalue:0, 	Line:
echo "SELECT * FROM Customers WHERE CustomerName LIKE 'a%!'(MISSING)"

@ruitianzhong
avoid printing confusing message which contains special character lik…
a49cefe 
…e `%`

Signed-off-by: ruitianzhong <ruitian-zhong@outlook.com>
@cfc4n
Copy link
Member
cfc4n commented Feb 29, 2024

Can you take a look at why golanglint did not detect the printf error?

https://github.com/gojue/ecapture/actions/runs/8090653564/job/22108477495?pr=495#step:6:22

@ruitianzhong
Copy link
Contributor Author
ruitianzhong commented Feb 29, 2024 •
edited
Loading

There are multiple linters in golangci-lint, such as errcheck,staticcheck,etc. To detect the error in this Pull Request, staticcheck linters must be enabled.
While in go-c-cpp.yml:

args: --disable-all -E errcheck

It disables all linter and enable errcheck only(-E option).

To enable staticcheck linter, it can be changed to :

args: --disable-all -E errcheck -E staticcheck

I ran golanglint-ci on 286bdcf again, and get the following output:

user/config/config_gotls.go:101:3: SA4006: this value of `err` is never used (staticcheck)
                err = fmt.Errorf("unsupport CPU arch :%s", goElfArch)
                ^
user/config/config_gotls.go:159:2: SA4006: this value of `err` is never used (staticcheck)
        offsets, err = gc.decodeInstruction(instHex)
        ^
user/event/event_gotls.go:32:3: SA4006: this value of `err` is never used (staticcheck)
                err = binary.Read(r, binary.LittleEndian, &ge.Data)
                ^
pkg/util/ebpf/bpf_test.go:39:2: SA4006: this value of `m` is never used (staticcheck)
        m, e := GetSystemConfig()
        ^
user/event/event_bash.go:65:7: SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck)
        s := fmt.Sprintf(fmt.Sprintf("PID:%d, UID:%d, \tComm:%s, \tRetvalue:%d, \tLine:\n%s", be.Pid, be.Uid, be.Comm, be.Retval, unix.ByteSliceToString((be.Line[:]))))
             ^
user/event/event_bash.go:70:7: SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck)
        s := fmt.Sprintf(fmt.Sprintf("PID:%d, UID:%d, \tComm:%s, \tRetvalue:%d, \tLine:\n%s,", be.Pid, be.Uid, be.Comm, be.Retval, dumpByteSlice([]byte(unix.ByteSliceToString((be.Line[:]))), "")))
             ^
user/event/event_mysqld.go:110:7: SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck)
        s := fmt.Sprintf(fmt.Sprintf(" PID:%d, Comm:%s, Time:%d,  length:(%d/%d),  return:%s, Line:%s", me.Pid, me.Comm, me.Timestamp, me.Len, me.Alllen, me.Retval, unix.ByteSliceToString((me.Query[:]))))

I verified through the GitHub Actions running on the code essentially only changed the configuration for workflow (i.e., add support for staticcheck only for Ubuntu 22.04 workflow which finally failed). links:https://github.com/ruitianzhong/ecapture/actions/runs/8094193279/job/22118377492#step:6:37

@ruitianzhong
add staticcheck for golangci-lint and fix related bugs
5b4a463 
Signed-off-by: ruitianzhong <ruitian-zhong@outlook.com>
@ruitianzhong
fix CI linter error
67a670c 
Signed-off-by: ruitianzhong <ruitian-zhong@outlook.com>
@ruitianzhong
safely remove empty branch
48eac9d 
Signed-off-by: ruitianzhong <ruitian-zhong@outlook.com>
@ruitianzhong
Copy link
Contributor Author
ruitianzhong commented Feb 29, 2024 •
edited
Loading

changes that do not change the program behavior

  • enable staticcheck linter to detect the printf error.
  • safely ignore err for kernel.HostVersion() because we have checked this in main function
  • remove unused variable/empty branch to pass static check

changes that change the program behavior

  • fix bug: correctly return err in user/config/config_gotls.go:101 and user/event/event_gotls.go:32

Finally, I want to mentioned the following code (previous) which is not obvious though :

var conf config.IConfig
	conf = goc
	if conf == nil {
	logger.Printf("ECAPTURE :: \tcant found module %s config info.", mod.Name())
		return
	}

conf is a concrete type and it is impossible to become nil for it. Instead we just check goc==nil first and have the other part unchanged. However, after checking goc,oc,gc and nc 's origin , it is found that they are seemmingly impossible to become nil.

Although the checks like goc==nil can be avoided, I still leave them intact. It's up to you to remove them safely with deeper understanding.

@ruitianzhong
add staticcheck for ubuntu-22.04
1d16026 
Signed-off-by: ruitianzhong <ruitian-zhong@outlook.com>
cfc4n
cfc4n approved these changes Feb 29, 2024
View reviewed changes
Copy link
Member
@cfc4n cfc4n left a comment •
edited
Loading

Choose a reason for hiding this comment

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

oops! The staticcheck configuration being turned off was my negligence.

good job, LGTM.

@cfc4n cfc4n merged commit b12878b into gojue:master Feb 29, 2024
6 checks passed
@ruitianzhong ruitianzhong deleted the bug-fix branch February 29, 2024 11:25
@cfc4n cfc4n added the bug Something isn't working label Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfc4n cfc4n cfc4n approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ruitianzhong @cfc4n