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
- Start a download and open its source list (
CSourceListCtrl).
- 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).
- 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.
CUpDownClient's five boolean secure-identification predicates disagree with theenum 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
amulethan inamuleguifor the same peer, and can render blank.Root cause
src/updownclient.h:658-661treats a missing credits object asIS_NOTAVAILABLE:The five predicates right below it (
src/BaseClient.cpp:2670-2693) instead guard oncredits &&, so withcredits == NULLevery one of them returnsfalse—including
SUINotSupported(), which is precisely the state the accessor above reports:creditsstarts asNULL(src/BaseClient.cpp:161) and is only assigned once thepeer'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 anif / else ifchain over those five predicates with no finalelse. It fillsIDC_CDIDENTin 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:
amuleamuleguisrc/BaseClient.cpp:2670-2693(credits && …)src/UpDownClientEC.h:142-155(m_identState == …)creditsNULLuntil the user hash resolvessrc/amule-remote-gui.cpp:2252)false→ label set to""→ blank fieldIS_NOTAVAILABLE→ "Not supported"The blank also overwrites the
_("N/A")placeholder the field is built with(
src/muuli_wdr.cpp:1236), becauseSetLabel("")is applied unconditionally.amuleguiis the correct side here: it reports what the core's ownGetCurrentIdentState()would report. The External Connection path already uses thataccessor (
src/ECSpecialCoreTags.cpp:381), which is whyamuleguiand the Web APIagree with each other and only the monolithic GUI is out of step.
Steps to reproduce
CSourceListCtrl).from a server response, so it has no user hash and therefore no credits object
(
src/GenericClientListCtrl.cpp:634opens the dialog).amule: Secure ident: is empty.amuleguiagainst the sameamuled: 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
NULLcase, sothere is one answer to "what is this peer's ident state":
This makes the monolithic build agree with
amuleguiand with the EC/Web API view, andremoves the duplicated
credits->GetCurrentIdentState(GetIP())call from each predicate.Independently of that,
GetSecureIdentTextStatus()should end with anelsethatreturns something (
_("N/A")or_("Unknown")), so an unmatched state can neversilently blank the field.
Related, same area
src/GenericClientListCtrl.cpp:902-910draws the SecIdent overlay forIsIdentified()('v') and
IsBadGuy()('X') but nothing forSUIFailed()— a peer whose signatureactually failed verification is visually indistinguishable from one that never
supported SecIdent.
IS_IDFAILEDarguably warrants more warning thanIS_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 bothstates; 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.