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

Bug categorization #181

Closed
unw9527 opened this issue Sep 19, 2022 · 2 comments
Closed

Bug categorization #181

unw9527 opened this issue Sep 19, 2022 · 2 comments
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@unw9527
Copy link
Contributor

unw9527 commented Sep 19, 2022

Basically, the two bullet points in each item follow the two questions @tylergu mentioned in Slack:

We need to think about how does the design of the operators lead to the bugs. Does the runtime or programming model make it easier to write these bugs? Or the developers are just careless?

What makes Acto to be able to find these bugs? Can we exploit some of the patterns to find more bugs?

Wrong logic for non-multiple-choice problems

Silently cast to default if invalid: (pingcap/tidb-operator#4650)

  • If there are two valid options A and B, developers will assume B is chosen if A is not. This is not a multiple-choice problem since developers have not limited the available input, while in the operator code, they assume that the input will be within a certain range of items.
  • Acto can find it because the input it generates is likely to be invalid if developers do not limit the available input in types.go using kubebuilder.

Wrong way of checking

Empty value: (pingcap/tidb-operator#4635)

  • Developers only check the length of a certain field (which is an array) to validate whether the input has all required fields specified. And if the length ≥ 3, they will consider it as valid input. However, it is possible that the input contains 3 empty items, which essentially mean nothing, and this array of empty items will bypass the validation.
  • Acto can find it because Acto generates the most basic value that can satisfy the requirement. Since the operator does not prohibit empty items in an array, Acto just does that.

Carelessness

Invalid image name: (cockroachdb/cockroach-operator#922)

  • We only have one case for this in crdb-operator. The developers assume that there is always a colon in the image name specified by the user. If there is no colon, the program will crash.
  • Acto can detect it because a colon is not required explicitly so the input it generates does not have a colon, which is invalid.

Required fields are not required explicitly: (pingcap/tidb-operator#4633)

  • More than 100 true alarms are caused by this. I think this is just because of the carelessness of developers.

    Think of this: if developers do either of the following work (which they should have done both), this bug will not show up:

    • explicitly require the presence of these required fields in types.go
    • add validation to check whether required fields are present or not

    Neither these two steps are done. So the input is rejected from the server side.

  • Acto can find it because the generated input does not have these required fields, which is invalid. In turn, pods will never be up.

Quantity conversion: (kubernetes/kubernetes#110654)

  • We found 3 bugs (all fixed now) in it. Most likely that developers are careless and do not take some extreme cases into consideration.
  • Acto generates many “nonsense” values (e.g. negative number, extremely large number, extremely long number) as input, which trigger bugs.

Find more bugs

  • From my perspective, quantity conversion is error-prone. We may find more bugs in it if we deliberately inject a lot of input to all fields which contain quantities. But I am not sure whether doing so conforms to Acto’s intention, as it sounds more like a fuzzing technique.
  • "if not A then B" seems to be a pattern which can be further explored: there are more than 1 place where TiDB developers apply this logic.
  • Another possible pattern is that when checking whether certain sub-fields are defined in a field of type array, developers only check the length of that array, without validating its content.
@unw9527 unw9527 self-assigned this Sep 19, 2022
@unw9527 unw9527 added the documentation Improvements or additions to documentation label Sep 19, 2022
@tylergu
Copy link
Member

tylergu commented Sep 20, 2022

More than 100 true alarms are caused by this
How many unique bugs are caused by this?

@unw9527
Copy link
Contributor Author

unw9527 commented Sep 21, 2022

How many unique bugs are caused by this?

One. See #144.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants