Skip to content

rabbit_auth_mechanism_plain:handle_response/2 fallback clause appears to be dead code #16259

Description

@lukebakken

What

rabbit_auth_mechanism_plain:handle_response/2 has a fallback clause for the case where the passed state is not a #state{} record:

handle_response(Response, #state{socket = Socket}) ->
    ...;
handle_response(Response, _State) ->
    case extract_user_pass(Response) of
        {ok, User, Pass} ->
            rabbit_access_control:check_user_pass_login(User, Pass);
        error ->
            {protocol_error, "response ~tp invalid", [Response]}
    end.

This fallback clause appears to be dead code under normal operation.

Why it looks dead

init/1 has a single clause that unconditionally returns #state{socket = Sock}:

init(Sock) ->
    #state{socket = Sock}.

Every caller I checked follows the usual AuthMechanism:init(Sock) followed by AuthMechanism:handle_response(Response, AuthState) cycle, where AuthState comes from init/1 and is therefore always a #state{} record:

  • rabbit_reader:auth_phase/2 (AMQP 0-9-1)
  • rabbit_amqp_reader:handle_sasl_frame/2 (AMQP 1.0)
  • rabbit_stream_reader:handle_frame_pre_auth/4 (RabbitMQ Stream Protocol)

A grep of the code base finds no call to rabbit_auth_mechanism_plain:handle_response/2 outside this pattern.

Origin

The fallback was added by 3bcdc0f as part of #13818 ("Fallback to original implementation of plain auth_mechanism if socket is not provided"). I cannot see how the _State clause is reached under the invariants the readers maintain today.

Ask

Decide which of the following applies and act accordingly:

  1. The fallback is genuinely defence in depth for a code path I have not found. In that case, add a short comment pointing to the call site that can invoke handle_response/2 without going through init/1, and add a test exercising it so it does not become silently unreachable.
  2. The fallback is dead code that can be removed without loss of behaviour, simplifying the module.

This came up as a secondary finding while investigating #16255 and its fix (PR #16258).

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions