Skip to content

The SUI* predicates contradict GetCurrentIdentState() when a client has no credits #727

Description

@ngosang

CUpDownClient's five boolean secure-identification predicates disagree with the
enum accessor on the same class about what "no credits object" means. As a result the
Secure ident: field of the Client Details dialog renders differently in the
monolithic amule than in amulegui for the same peer, and can render blank.

Root cause

src/updownclient.h:658-661 treats a missing credits object as IS_NOTAVAILABLE:

EIdentState GetCurrentIdentState() const
{
    return credits ? credits->GetCurrentIdentState(GetIP()) : IS_NOTAVAILABLE;
}

The five predicates right below it (src/BaseClient.cpp:2670-2693) instead guard on
credits &&, so with credits == NULL every one of them returns false
including SUINotSupported(), which is precisely the state the accessor above reports:

bool CUpDownClient::SUINotSupported() const
{
    return (credits && credits->GetCurrentIdentState(GetIP()) == IS_NOTAVAILABLE);
}

credits starts as NULL (src/BaseClient.cpp:161) and is only assigned once the
peer's user hash resolves (src/BaseClient.cpp:684-697), so this is a reachable state,
not a theoretical one.

Observable effect

CClientRef::GetSecureIdentTextStatus() (src/ClientRef.cpp:173-192) is an
if / else if chain over those five predicates with no final else. It fills
IDC_CDIDENT in the Client Details dialog (src/ClientDetailDialog.cpp:167).

GUI_SOURCES (cmake/source-vars.cmake:80) compiles this dialog into both binaries,
but the predicates behind it are different implementations:

monolithic amule amulegui
Predicates src/BaseClient.cpp:2670-2693 (credits && …) src/UpDownClientEC.h:142-155 (m_identState == …)
credits NULL until the user hash resolves always allocated (src/amule-remote-gui.cpp:2252)
Peer without credits all five false → label set to ""blank field IS_NOTAVAILABLE"Not supported"

The blank also overwrites the _("N/A") placeholder the field is built with
(src/muuli_wdr.cpp:1236), because SetLabel("") is applied unconditionally.

amulegui is the correct side here: it reports what the core's own
GetCurrentIdentState() would report. The External Connection path already uses that
accessor (src/ECSpecialCoreTags.cpp:381), which is why amulegui and the Web API
agree with each other and only the monolithic GUI is out of step.

Steps to reproduce

  1. Start a download and open its source list (CSourceListCtrl).
  2. Double-click a source that has not completed the eD2k handshake yet — one added
    from a server response, so it has no user hash and therefore no credits object
    (src/GenericClientListCtrl.cpp:634 opens the dialog).
  3. Monolithic amule: Secure ident: is empty. amulegui against the same
    amuled: Secure ident: reads "Not supported".

The window is narrow — a source usually acquires credits or is dropped quickly — which
is presumably why this has gone unnoticed.

Suggested fix

Route the five predicates through the accessor that already handles the NULL case, so
there is one answer to "what is this peer's ident state":

bool CUpDownClient::IsIdentified() const     { return GetCurrentIdentState() == IS_IDENTIFIED; }
bool CUpDownClient::IsBadGuy() const         { return GetCurrentIdentState() == IS_IDBADGUY; }
bool CUpDownClient::SUIFailed() const        { return GetCurrentIdentState() == IS_IDFAILED; }
bool CUpDownClient::SUINeeded() const        { return GetCurrentIdentState() == IS_IDNEEDED; }
bool CUpDownClient::SUINotSupported() const  { return GetCurrentIdentState() == IS_NOTAVAILABLE; }

This makes the monolithic build agree with amulegui and with the EC/Web API view, and
removes the duplicated credits->GetCurrentIdentState(GetIP()) call from each predicate.

Independently of that, GetSecureIdentTextStatus() should end with an else that
returns something (_("N/A") or _("Unknown")), so an unmatched state can never
silently blank the field.

Related, same area

src/GenericClientListCtrl.cpp:902-910 draws the SecIdent overlay for IsIdentified()
('v') and IsBadGuy() ('X') but nothing for SUIFailed() — a peer whose signature
actually failed verification is visually indistinguishable from one that never
supported SecIdent. IS_IDFAILED arguably warrants more warning than IS_IDBADGUY,
which the code itself notes is transient for 1-2 seconds after a peer IP change
(src/ClientCredits.cpp:216-218). The Web UI now shows the warning icon for both
states; the wx client lists could do the same.

Impact

Cosmetic, and the reproduction window is short. Filing it for the underlying
contradiction rather than the symptom: two accessors on the same class currently give
different answers to the same question, and the boolean set is the one that cannot
express a state the enum set can.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions