ClientManager::findLegacyUser(), assertion "aNick.size() > 0" fails

[qwertitis]

I have no idea why this happened. I was doing some testing (tried and true random clicking) on some Russian hub, and I can't make it happen again. The address of the hub was lost when the application failed assertion, unfortunately.

linuxdcpp: dcpp/ClientManager.cpp:187: dcpp::UserPtr dcpp::ClientManager::findLegacyUser(const std::string&) const: Försäkran "aNick.size() > 0" falsk.

[Steven Sheehy (steven-sheehy)]

Please provide more information so we can research it. Please also reproduce on trunk to rule out any issues with bugs introduced into branch.

[E_zombie (lv77)]

revno: 325 crash to

linuxdcpp: dcpp/ClientManager.cpp:186: dcpp::UserPtr dcpp::ClientManager::findLegacyUser(const std::string&) const: Проверочное утверждение «aNick.size() > 0» не выполнено.
Аварийный останов (core dumped)

fedora 11
start search on nmdc+ADC( adc://adc2.san.ru:10000 ) hub after 1 day uptime

core+linuxdcpp
ftp://ftp.san.ru/temp/crash-linuxdcpp/2009091801/

[Steven Sheehy (steven-sheehy)]

I'm not sure why you mention an ADC hub, the backtrace clearly shows it is a NMDC command called $SR for search results that is causing it to fail. ADC would use a RES command and would not invoke the flow that could cause a failure in ClientManager::findLegacyUser.

Since it's NMDC, most likely it's crashing because a user in the channel was using a nick in an encoding different than the one you specified. Also, it will only crash with a debug build due to the assertion not being present in non-debug builds. However, we should probably put a check in SearchManager::onData() to verify the conversion of the $SR parameters to utf-8 was successful before using them.

[Steven Sheehy (steven-sheehy)]

Backtrace from provided core file:

(gdb) bt full
#0 0x0072f416 in __kernel_vsyscall ()
No symbol table info available.
#1 0x0081e7c1 in ?? ()
No symbol table info available.
#2 0x00820092 in ?? ()
No symbol table info available.
#3 0x008178ee in ?? ()
No symbol table info available.
#4 0x08146f42 in dcpp::ClientManager::findLegacyUser (this=0x84c41f8,
    aNick=@0xb0ffe160) at dcpp/ClientManager.cpp:186
 l = {cs = @0x84c4244}
 __PRETTY_FUNCTION__ = "dcpp::UserPtr dcpp::ClientManager::findLegacyUser(const std::string&) const"
#5 0x081cc2ed in dcpp::SearchManager::onData (this=0x84c4088,
    buf=0xb1a013d8 "$SR ������� �������\\Photoshop_CS3_Rus_Portable-�������\\PS\\Presets\\Zoomify\\Zoomify Viewer with Navigator (Gray Background).zvt\0051799 0/5\005TTH:R4JZ3FOWDYTG6EEC3QJKRU646B3EPPYM5NMAZ6Q (88.147.128.10 :411)|"...,
    aLen=200, remoteIp=@0xb0ffe248) at dcpp/SearchManager.cpp:228
 i = 180
 cnt = 2
 file = {static npos = 4294967295,
  _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>},
    _M_p = 0x9f5248b4 "\\Photoshop_CS3_Rus_Portable-\\PS\\Presets\\Zoomify\\Zoomify Viewer with Navigator (Gray Background).zvt"}}
 freeSlots = 0
 slots = 5
 hubIpPort = {static npos = 4294967295,
  _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0x9f52346c "88.147.128.10 :411"}}
 j = 198
 size = 1799
 type = dcpp::SearchResult::TYPE_FILE
 url = {static npos = 4294967295,
  _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0x37ee9c ""}}
 nick = {static npos = 4294967295,
  _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0x9f522f7c ""}}
 hubName = {static npos = 4294967295,
  _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>},
    _M_p = 0x9f524494 "TTH:R4JZ3FOWDYTG6EEC3QJKRU646B3EPPYM5NMAZ6Q"}}
 encoding = {static npos = 4294967295,
  _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0x84bcc5c "UTF-8"}}
 user = {p_ = 0x0}
 tth = {static npos = 4294967295,
  _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0xb0ffeb40 "95.84.13.234"}}
 sr = {p_ = 0x9f52341c}
 x = {static npos = 4294967295,
  _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>},
    _M_p = 0x9f52469c "$SR ������� �������\\Photoshop_CS3_Rus_Portable-�������\\PS\\Presets\\Zoomify\\Zoomify Viewer with Navigator (Gray Background).zvt\0051799 0/5\005TTH:R4JZ3FOWDYTG6EEC3QJKRU646B3EPPYM5NMAZ6Q (88.147.128.10 :411)|"}}
#6 0x081cb65a in dcpp::SearchManager::run (this=0x84c4088)
    at dcpp/SearchManager.cpp:114
 failed = false
 buf = {
  ptr = 0xb1a013d8 "$SR ������� �������\\Photoshop_CS3_Rus_Portable-�������\\PS\\Presets\\Zoomify\\Zoomify Viewer with Navigator (Gray Background).zvt\0051799 0/5\005TTH:R4JZ3FOWDYTG6EEC3QJKRU646B3EPPYM5NMAZ6...

[Steven Sheehy (steven-sheehy)]

To fix I suggest we either validate the utf8 using Text::validateUtf8() or just check if the params are non-empty and return if they are inside SearchManager::onData(). What approach does the DC++ team prefer?

[Steven Sheehy (steven-sheehy)]

I committed a fix to dc++ (linuxdcpp fix to come later). This fix simply checks if the params are non-empty inside SearchManager before continuing. What'd I'd prefer to do is validate that the text is utf-8, but I don't think SearchManager is the proper place for that. I think from a design perspective it makes more sense for NmdcHub to handle the conversion of all NMDC commands. I propose that we consolidate all of our conversion and validation logic inside the beginning of NmdcHub::onLine() similar to how we now validate ADC commands (see bug #300268). Right now the conversion to utf8 is done in multiple places inside onLine() and is sometimes even delegated to other classes as in the case with SearchManager. As a result, SearchManager has to lookup the hub's encoding separately inside onData() whereas if we did it in NmdcHub it would already be aware of the encoding. Also, right now we do not validate that the text was converted successfully as we do with ADC. I recommend we both convert and validate the entire command at the beginning of NmdcHub::onLine(). Arne/Poy thoughts?

[Jacek Sieka (arnetheduck)]

Actually, the nick in nmdc should be treated as a binary blob and kept separate from the display nick (which probably should be converted disregarding encoding errors...)...then findlegacy would not have to bother with encodings at all...
converting everything to utf8 in nmdc is not very friendly towards other clients / legacy users.
we've covered this before in other contexts I believe, but noone's cared enough to actually fix it...

[Steven Sheehy (steven-sheehy)]

Right, I agree that nicks and filenames should be treated as binary except for display (bug #351208), but my point was that we should validate any nmdc text we convert to utf-8 is actually non-empty and utf-8 before using it. My proposed solution was perhaps not the greatest, but that's of course changeable. Note that I was not proposing that we should send utf-8 to other nmdc clients, only that we should convert and validate received nmdc commands to utf-8 before handling them (right now we only convert).

My other point was that right now the conversion of nmdc params to utf-8 is spread around in other classes. I think it would be better to encapsulate this conversion inside NmdcHub so that data can be converted and validated as soon as it comes down the tubes and so that other classes can focus on their designated role instead of worrying about text conversion/validation.

I should also note that the peer can send a space or empty string for nick in $SR and still crash debug builds, so the fix I committed would still be needed regardless of encoding issues. Perhaps we should just remove the dcassert?

[Jacek Sieka (arnetheduck)]

Removing the dcassert sounds fine
Converting in nmdchub sounds fine as long as the binary nick thing is done
Validating the utf8 meant for display also sounds fine, assuming we replace any bad chars as opposed to discarding the whole message...or at least that seems like the most reasonable tradeoff unless you have a better idea?

I don't remember the code but I'm guessing most out-of-nmdc-conversions are done either in protocolspecific bits (search results?) or would go away were the binary nick thing implemented...in any case keeping it in nmdchub makes a lot of sense.

[poy (poy)]

Fixed in version 0.760.