Migrate codebase to use S2N_RESULT by default
- Dominant language
- C
- Stars
- 4.8k
- Forks
- 802
- Avg merge
- 5d 16h
- Merged PRs (30d)
- 27
Description
### Problem:
Right now, most of the codebase uses `int` as the function return value. As documented in [s2n_result.c](https://github.com/awslabs/s2n/blob/main/utils/s2n_result.c), this has a few problems:
* `GUARD`ing _in_ a function that returns integer types
* `GUARD`ing a function that returns integer type
* Forgetting to `GUARD` a function that returned an error signal
### Solution:
The majority of the codebase should return `S2N_RESULT`. This is statically checked to ensure:
* The code cannot `GUARD` in a function that returns integer types:
```c
uint8_t s2n_answer_to_the_ultimate_question() {
GUARD(s2n_sleep_for_years(7500000)); /* <- Won't compile since this function doesn't return an S2N_RESULT */
return 42;
}
```
* The code cannot `GUARD` a function that returns integer types:
```c
S2N_RESULT s2n_deep_thought() {
GUARD(s2n_answer_to_the_ultimate_question()); /* <- Won't compile since the function being called doesn't return an S2N_RESULT */
return S2N_RESULT_OK;
}
```
* The code cannot ignore the return value of a function
```c
uint8_t s2n_answer_to_the_ultimate_question() {
s2n_sleep_for_years(7500000); /* <- Won't compile since the function being called returns a `S2N_RESULT` isn't `GUARD`ed */
return 42;
}
```
### Requirements / Acceptance Criteria:
The following tasks are implemented in a way that will make the transition as painless as possible, especially for any pending PRs.
- [x] Implement S2N_RESULT #1872
- [x] Add a codegen script to consistently generate safety macros for all function contexts #2423
- [x] Add a codemod script to migrate all of the existing code to use the new naming convention #2339
- [x] Implement a GitHub Action to ensure the s2n_safety_macros.h is _only_ modified through the codegen script. (see https://github.com/awslabs/s2n/pull/2423#discussion_r533657339)
- [x] Add a `S2N_RESULT_FREE` return type that is allowed to be used in the `DEFER_CLEANUP` macro. #2523
- [x] Apply the codemod script to main and existing PRs #2441
- [x] Remove the old safety macros, since the codemod has been applied and none of the code is using the old macros anymore. #2747
- [ ] Update the safety macros codegen script to add prefix-less set of macros by duplicating all of the `RESULT_` declarations. This means that `RESULT_GUARD` is now just `GUARD`.
- [ ] Add a codemod script to move all of the `RESULT_` invocations in the codebase, since it's now the default.
- [ ] Manually update all of the functions in the codebase to return `S2N_RESULT` instead of `int` (see #1891 #2371)
This can be done per module and replacing all of the `int` with `S2N_RESULT` and trying to compile it. After working through all of the compiler issues, that module should continue to have the same functionality with the added guarantees that `S2N_RESULT` provides.
- [ ] Improve comparison macros to ensure values have the same type before comparing
Contributor guide
Research direction
Start with utils/s2n_result.c and the generated s2n_safety_macros.h to understand S2N_RESULT and the existing safety macros. Review the remaining unchecked tasks, including the codegen and codemod work and the manual module-by-module migration from int. Done requires the relevant migration tasks to compile cleanly while preserving existing functionality and completing the stated comparison-macro improvements.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c
- Domain
- security
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100