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

Fix syncd crashing due to invalid length being used for string printing #806

Conversation

saiarcot895
Copy link
Contributor

@saiarcot895 saiarcot895 commented Jul 28, 2023

PR #801 added some code to convert binary strings to be printable strings. However, one code path used the raw value of len without sanity checking as the length of the string, when the length() function should have been used instead.

Sample syncd backtrace:

#0  0x00007fddc635fce1 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x00007fddc6349537 in abort () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x00007fddc65af7ec in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#3  0x00007fddc65ba966 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#4  0x00007fddc65ba9d1 in std::terminate() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#5  0x00007fddc65bac65 in __cxa_throw () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#6  0x00007fddc65b209a in std::__throw_length_error(char const*) () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#7  0x00007fddc664849c in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long) () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#8  0x00007fddc6648cfc in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::reserve(unsigned long) () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#9  0x00007fddc6daad6c in swss::binary_to_printable (buffer=0x55feeaced250, length=18446744073354011680) at common/stringutility.h:172
#10 0x00007fddc6dac41c in swss::RedisCommand::toPrintableString[abi:cxx11]() const (this=0x7ffc0af84500) at common/rediscommand.cpp:133
#11 0x00007fddc6d195b1 in swss::RedisReply::RedisReply (this=0x7ffc0af84508, ctx=0x55feeace8ca0, command=...) at common/redisreply.cpp:94
#12 0x00007fddc6d19bab in swss::RedisReply::RedisReply (this=0x7ffc0af84508, ctx=0x55feeace8ca0, command=..., expectedType=1) at common/redisreply.cpp:116
#13 0x000055fee92c9dcd in syncd::RedisClient::RedisClient(std::shared_ptr<swss::DBConnector>) ()
#14 0x000055fee9280681 in syncd::Syncd::Syncd(std::shared_ptr<sairedis::SaiInterface>, std::shared_ptr<syncd::CommandLineOptions>, bool) ()
#15 0x000055fee92697ed in syncd_main(int, char**) ()
#16 0x000055fee9267b8f in main ()

#10 0x00007fddc6dac41c in swss::RedisCommand::toPrintableString[abi:cxx11]() const (this=0x7ffc0af84500) at common/rediscommand.cpp:133
133     common/rediscommand.cpp: No such file or directory.
(gdb) print *this
$22 = {temp = 0x55feeaced250 "*3\r\n$6\r\nSCRIPT\r\n$4\r\nLOAD\r\n$863\r\nlocal keys = redis.call('KEYS', KEYS[1])\nlocal n = table.getn(keys)\n\nfor i = 1, n do\n   if KEYS[2] == \"\" and KEYS[3] == \"1\" then\n      redis.call('DEL', keys[i])  \n   e"..., len = -355539936}

Fix this by using the length() function.

PR sonic-net#801 added some code to convert binary strings to be printable
strings. However, one code path used the raw value of `len` without
sanity checking as the length of the string, when the `length()`
function should have been used instead.

Sample syncd backtrace:

```

(gdb) print *this
$22 = {temp = 0x55feeaced250 "*3\r\n$6\r\nSCRIPT\r\n$4\r\nLOAD\r\n$863\r\nlocal keys = redis.call('KEYS', KEYS[1])\nlocal n = table.getn(keys)\n\nfor i = 1, n do\n   if KEYS[2] == \"\" and KEYS[3] == \"1\" then\n      redis.call('DEL', keys[i])  \n   e"..., len = -355539936}
```

Fix this by using the `length()` function.

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>
@@ -130,7 +130,7 @@ int RedisCommand::appendTo(redisContext *ctx) const

std::string RedisCommand::toPrintableString() const
{
return binary_to_printable(temp, len);
return binary_to_printable(temp, length());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

length

Add a unit test?

@qiluo-msft
Copy link
Contributor

    throw std::bad_alloc();

Is it possible outside swallow this exception, and keep a negative len in the object?


Refers to: common/rediscommand.cpp:34 in 5f71661. [](commit_id = 5f71661, deletion_comment = False)

@qiluo-msft
Copy link
Contributor

    throw std::bad_alloc();

The return value is actually length or error code in concept, we need to translate it to proper len.


In reply to: 1656337880


Refers to: common/rediscommand.cpp:34 in 5f71661. [](commit_id = 5f71661, deletion_comment = False)

@qiluo-msft
Copy link
Contributor

if (len == -1) {

The same


Refers to: common/rediscommand.cpp:49 in 5f71661. [](commit_id = 5f71661, deletion_comment = False)

@saiarcot895
Copy link
Contributor Author

Closing as #807 fixes the actual bug. This issue is due to mixing of binaries that had different struct sizes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants