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

C++ ODR Violations #2137

Closed
fosterbrereton opened this issue Oct 12, 2022 · 2 comments
Closed

C++ ODR Violations #2137

fosterbrereton opened this issue Oct 12, 2022 · 2 comments
Labels
bug This issue is a bug. dependencies This issue is a problem in a dependency. investigating This issue is being investigated and/or work is in progress to resolve the issue. p2 This is a standard priority issue

Comments

@fosterbrereton
Copy link

fosterbrereton commented Oct 12, 2022

Describe the bug

Loosely stated, the One Definition Rule in C++ requires that for any given symbol in an application, there must be one and only one definition of it. Compilers and linkers assume this rule to hold for client code, and the Standard does not require that tooling detect or report violations of ODR. The effects of ODRVs in code vary from malignant (obscure crashes) to benign (the debugger "lying" to developers).

Using Adobe ORC we have detected 3 ODRVs in the AWS sources. Each of these definitions are unlikely to cause runtime issues because the structures are defined and used within singular source files. Nevertheless, they are multiple, varying definitions of singular symbols, which are likely to at least cause debugging issues for clients, if not something worse.

Note that you cannot simply run the ORC tool over the final, high-level AWS SDK library/libraries. Because of the way linkers work, any ODR violations will be embedded within the final product and be undetectable. It's only when the individual components are being brought together at link time when the violations can be found.

Detected violations:

Symbol decoder_state

In library libaws-c-compression.a,
in source file aws-crt-cpp/crt/aws-c-compression/source/huffman.c:191:

struct decoder_state {
    struct aws_huffman_decoder *decoder;
    struct aws_byte_cursor *input_cursor;
};

In library libaws-c-http.a,
in source file crt/aws-crt-cpp/crt/aws-c-http/source/h2_decoder.c:129:

struct decoder_state {
    state_fn *fn;
    uint32_t bytes_required;
    const char *name;
};

Symbol imds_user_data

In library libaws-c-auth.a,
in source file crt/aws-crt-cpp/crt/aws-c-auth/source/credentials_provider_imds.c:110:

struct imds_user_data {
    /* immutable post-creation */
    struct aws_allocator *allocator;
    struct aws_credentials_provider *imds_provider;
    aws_on_get_credentials_callback_fn *original_callback;
    struct aws_byte_buf role;
    void *original_user_data;
};

In library libaws-c-auth.a,
in source file crt/aws-crt-cpp/crt/aws-c-auth/source/aws_imds_client.c:209:

struct imds_user_data {
    /* immutable post-creation */
    struct aws_allocator *allocator;
    struct aws_imds_client *client;
    aws_imds_client_on_get_resource_callback_fn *original_callback;
    void *original_user_data;

    /* mutable */
    struct aws_http_connection *connection;
    struct aws_http_message *request;
    struct aws_byte_buf current_result;
    struct aws_byte_buf imds_token;
    struct aws_string *resource_path;
    struct aws_retry_token *retry_token;
    /*
     * initial value is copy of client->token_required,
     * will be adapted according to response.
     */
    bool imds_token_required;
    bool is_imds_token_request;
    int status_code;
    int error_code;

    struct aws_atomic_var ref_count;
};

Symbol write_request

In libaws-c-io.a,
in source file crt/aws-crt-cpp/crt/aws-c-io/source/posix/socket.c:1298:

struct write_request {
    struct aws_byte_cursor cursor_cpy;
    aws_socket_on_write_completed_fn *written_fn;
    void *write_user_data;
    struct aws_linked_list_node node;
    size_t original_buffer_len;
    int error_code;
};

In libaws-c-io.a,
in source file crt/aws-crt-cpp/crt/aws-c-io/source/posix/pipe.c:49:

struct write_request {
    struct aws_byte_cursor original_cursor;
    struct aws_byte_cursor cursor; /* tracks progress of write */
    size_t num_bytes_written;
    aws_pipe_on_write_completed_fn *user_callback;
    void *user_data;
    struct aws_linked_list_node list_node;

    /* True if the write-end is cleaned up while the user callback is being invoked */
    bool did_user_callback_clean_up_write_end;
};

Expected Behavior

No ODR violations in the AWS C++ SDK.

Current Behavior

Violations are being found.

Reproduction Steps

  • Build and run Adobe ORC over the AWS C++ SDK's individual libraries, as they are used to generate the final SDK components.
  • Analyze the report

Possible Solution

Rename the symbols such that they are unique across the entire AWS SDK library. In general, the use of C++ namespaces goes a long way towards reducing ODR violations (especially source-level anonymous namespaces.) If the files are C-based, then altering the name of the symbol is generally the only path forward. Note, too, that non-namespaced symbols may also conflict with symbols defined by clients using the AWS C++ SDK, so taking efforts to make the SDK symbol names unique is important.

Additional Information/Context

No response

AWS CPP SDK version used

1.9.350

Compiler and Version used

Apple clang version 13.1.6 (clang-1316.0.21.2.5)

Operating System and version

macOS 12.0.1 (Monterey)

@fosterbrereton fosterbrereton added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 12, 2022
@jmklix
Copy link
Member

jmklix commented Oct 13, 2022

Thanks for bringing this up. We are looking into fixing this for the dependencies that you have listed

@jmklix jmklix added dependencies This issue is a problem in a dependency. and removed needs-triage This issue or PR still needs to be triaged. labels Oct 13, 2022
@jmklix jmklix added needs-review This issue or pull request needs review from a core team member. p2 This is a standard priority issue labels Nov 4, 2022
@graebm graebm added the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Nov 8, 2022
graebm added a commit to awslabs/aws-c-io that referenced this issue Nov 8, 2022
**Issue:**
aws/aws-sdk-cpp#2137

**Description of Changes:**
Rename private `write_request` struct avoid collisions with same-named struct in other .c files
graebm added a commit to awslabs/aws-c-compression that referenced this issue Nov 8, 2022
**Issue:**
aws/aws-sdk-cpp#2137

**Description of Changes:**
Rename `decoder_state` -> `huffman_decoder_state` to avoid collision with same-named struct in aws-c-http.
graebm added a commit to awslabs/aws-c-http that referenced this issue Nov 8, 2022
**Issue:**
aws/aws-sdk-cpp#2137

**Description of Changes:**
Rename `decoder_state` -> `h2_decoder_state` to avoid collision with same-named struct in aws-c-compression.
graebm added a commit to awslabs/aws-c-auth that referenced this issue Nov 8, 2022
**Issue:**
aws/aws-sdk-cpp#2137

**Description of Changes:**
Rename `imds_user_data` -> `imds_provider_user_data` in `credentia_provider_imds.c`, to avoid collision with same-named struct in `aws_imds_client.c`.
graebm added a commit to awslabs/aws-c-io that referenced this issue Nov 8, 2022
**Issue:**
aws/aws-sdk-cpp#2137

**Description of Changes:**
Rename private `write_request` struct avoid collisions with same-named struct in other .c files
graebm added a commit to awslabs/aws-c-compression that referenced this issue Nov 8, 2022
**Issue:**
aws/aws-sdk-cpp#2137

**Description of Changes:**
Rename `decoder_state` -> `huffman_decoder_state` to avoid collision with same-named struct in aws-c-http.
graebm added a commit to awslabs/aws-c-http that referenced this issue Nov 8, 2022
**Issue:**
aws/aws-sdk-cpp#2137

**Description of Changes:**
Rename `decoder_state` -> `h2_decoder_state` to avoid collision with same-named struct in aws-c-compression.
graebm added a commit to awslabs/aws-c-auth that referenced this issue Nov 9, 2022
**Issue:**
aws/aws-sdk-cpp#2137

**Description of Changes:**
Rename `imds_user_data` -> `imds_provider_user_data` in `credentia_provider_imds.c`, to avoid collision with same-named struct in `aws_imds_client.c`.
graebm added a commit to awslabs/aws-crt-cpp that referenced this issue Nov 9, 2022
**Issue:**
aws/aws-sdk-cpp#2137

**Description of changes:**
Use latest versions of submodules, which have fixed their ODR violations
@jmklix jmklix removed the needs-review This issue or pull request needs review from a core team member. label Nov 18, 2022
@graebm graebm closed this as completed Sep 21, 2023
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. dependencies This issue is a problem in a dependency. investigating This issue is being investigated and/or work is in progress to resolve the issue. p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

3 participants