acw/simple_asn1
1
0
Fork 0
Code Issues 5 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

Return error instead of panics on invalid ASN.1 #9

Merged
Flakebi merged 4 commits from master into master 2019-03-29 16:20:13 -07:00
Conversation 4 Commits 4 Files Changed 4 +123 -29
Flakebi commented 2019-03-29 10:53:58 -07:00 (Migrated from github.com)
Copy Link

Hi,
I use this library to find out if something is ASN.1 or not, unfortunately it often panics on invalid input.
To find all panics I added a cargo-fuzz project and fixed all the panics it found.
With the final version, the fuzzer did not find any panic for half an hour.

Unfortunately this pr currently includes a breaking change because it adds error variants. So I’m not sure if you want to implement this someway else, e.g. return some of the existing errors instead.

Edit: I found this commit which also adds a + Error bound to the error types.

Hi, I use this library to find out if something is ASN.1 or not, unfortunately it often panics on invalid input. To find all panics I added a [cargo-fuzz](https://fuzz.rs/book/) project and fixed all the panics it found. With the final version, the fuzzer did not find any panic for half an hour. Unfortunately this pr currently includes a breaking change because it adds error variants. So I’m not sure if you want to implement this someway else, e.g. return some of the existing errors instead. Edit: I found [this commit](https://github.com/mxork/simple_asn1/commit/4f16e859502ccb1e1ddf90f7e9e311d18fcf1573) which also adds a `+ Error` bound to the error types.
acw commented 2019-03-29 14:42:24 -07:00 (Migrated from github.com)
Copy Link

This is awesome, and I'm always happy to add more (and more helpful) error variants. Let me check a couple things and then I'll merge it.

This is awesome, and I'm always happy to add more (and more helpful) error variants. Let me check a couple things and then I'll merge it.
Flakebi commented 2019-03-29 14:49:35 -07:00 (Migrated from github.com)
Copy Link

Thanks!

One thing I forgot to mention: The GeneralizedTime parser previously inserted the dot at position 15, 14 should be the right position :)

Thanks! One thing I forgot to mention: The GeneralizedTime parser [previously](https://github.com/acw/simple_asn1/pull/9/files#diff-b4aea3e418ccdb71239b96952d9cddb6L535) inserted the dot at position 15, 14 *should* be the right position :)
acw commented 2019-03-29 15:52:19 -07:00 (Migrated from github.com)
Copy Link

Yes, I saw that and that was one of the things I wanted to look at (and maybe write a test case for). :)

Yes, I saw that and that was one of the things I wanted to look at (and maybe write a test case for). :)
acw commented 2019-03-29 16:35:34 -07:00 (Migrated from github.com)
Copy Link

This version has been pushed as 0.4.0.

This version has been pushed as 0.4.0.
❤️ 1
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
bug
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
acw
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: acw/simple_asn1#9
Delete Branch "master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?