Skip to content

Dev.libfuzzer #1

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

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

Dev.libfuzzer #1

wants to merge 3 commits into from

Conversation

kyakdan
Copy link
Member

@kyakdan kyakdan commented Dec 30, 2021

This PR will be imported into Gerrit with the title and first
comment (this text) used to generate the subject and body of
the Gerrit change.

Please ensure you adhere to every item in this list.

More info can be found at https://github.com/golang/go/wiki/CommitMessage

  • The PR title is formatted as follows: net/http: frob the quux before blarfing
    • The package name goes before the colon
    • The part after the colon uses the verb tense + phrase that completes the blank in,
      "This change modifies Go to ___________"
    • Lowercase verb after the colon
    • No trailing period
    • Keep the title as short as possible. ideally under 76 characters or shorter
  • No Markdown
  • The first PR comment (this one) is wrapped at 76 characters, unless it's
    really needed (ASCII art, table, or long link)
  • If there is a corresponding issue, add either Fixes #1234 or Updates #1234
    (the latter if this is not a complete fix) to this comment
  • If referring to a repo other than golang/go you can use the
    owner/repo#issue_number syntax: Fixes golang/tools#1234
  • We do not use Signed-off-by lines in Go. Please don't add them.
    Our Gerrit server & GitHub bots enforce CLA compliance instead.
  • Delete these instructions once you have read and applied them

trampoline is only implemented for amd64
 This provides more detailed information about the fuzzing
 process including the basic blocks that were covered by fuzzing
@kyakdan kyakdan requested a review from fmeum December 30, 2021 16:04
Copy link

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

I think that we should maybe split this into three individual PRs, possibly in this order: The 8-bit counters, the string compares, and the improved int compares with trampoline. The first two will be much easier to review and could be used by us to test the waters and get to know the process before the last one. What do you think?

Copy link

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

I left one last comment.

Looks really good, now we only need to split it up into three PRs with good commit messages. If you want, we can iterate on them in a doc.

data += strconv.FormatInt(int64(n.Pos().FileIndex()), 10)
data += n.Pos().LineNumber()
data += strconv.FormatUint(uint64(n.Pos().Col()), 10)
hash := sha1.Sum([]byte(data))
Copy link

Choose a reason for hiding this comment

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

Rather than using sha1.Sum, we should probably create a Hash instance and write data to it, which requires no conversion to strings or allocations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Writing data to a Hash instance might return an error. How would you generate IDs in these cases?

Copy link

Choose a reason for hiding this comment

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

While the io.Writer interface might in general, Hash explicitly says that it never returns an error, so you can simply ignore it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've overseen that comment. Will change it.

_ = binary.Write(hash, binary.LittleEndian, int64(n.Pos().Col()))
// We also include the string representation of the node to distinguish autogenerated expression since
// those get the same `src.XPos`
_, _ = io.WriteString(hash, fmt.Sprintf("%v", n))
Copy link

Choose a reason for hiding this comment

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

These nodes seem to have AutoTmp() set and you may be able to get the name from .Name() directly. We should try not to rely on "%v".

Copy link
Member Author

Choose a reason for hiding this comment

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

Some nodes such as ir.BinaryExpr do not have a name and we would need to traverse the children to get the names. What would be the disadvantages of relying on "%v"

Copy link

Choose a reason for hiding this comment

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

I see, it's probably better than the alternatives then. Do we still have to hash in the other fields or are they also part of the string representation?

kyakdan pushed a commit that referenced this pull request May 17, 2022
rand.Prime does not guarantee the precise prime selection algorithm as
part of its contract. For example, it changed slightly in CL 387554. We
want to ensure that no tests come to rely on it staying the same, so
just like other cryptographic functions that use randomness in an
unspecified way (ECDSA signing, RSA PKCS #1 v1.5 encryption, RSA key
generation), make it randomly read an extra byte or not.

Change-Id: Ib9079c03360812d412b7c21d5a06caadabb4a8bf
Reviewed-on: https://go-review.googlesource.com/c/go/+/391554
Run-TryBot: Filippo Valsorda <[email protected]>
Auto-Submit: Filippo Valsorda <[email protected]>
Trust: Filippo Valsorda <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants