aws / aws/s2n-tls

Migrate codebase to use S2N_RESULT by default

Open
#2,425 0 comments 0 reactions 0 assignees View on GitHub
ops-backlog priority/low s2n-core size/large
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.