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

msg/async: fix bug of data type conversion when uint64_t -> int -> uint64_t #18210

Merged
merged 1 commit into from Oct 12, 2017

Conversation

shangfufei
Copy link

There is "uint64_t a -> int b ->uint64_t c" data type conversion in the code, when the length of "a" is greater than 32 bits, the conversion of "uint64_t a" to "int b" results in data overflow, when the length of "a" is less than 32 bits and a >= 0x80000000, change "uint64_t a" to "int b" will lead "b" to be a negative number of 32 bits, then change " int b" to " uint64_t c" , the variable "c" is not equal to the variable “a” at this situation.

Signed-off-by: shangfufei shangfufei@inspur.com

@yuyuyu101
Copy link
Member

who is "int"

@shangfufei
Copy link
Author

class C_tick_wakeup : public EventCallback {
AsyncConnectionRef conn;

public:
explicit C_tick_wakeup(AsyncConnectionRef c): conn(c) {}
void do_request(int fd_or_id) {
conn->tick(fd_or_id);
}
};

fd_or_id is int
time_event_next_id is uint64_t

@shangfufei
Copy link
Author

It got assert(last_tick_id == id) in our code when EventCenter::process_time_events cb->do_request(int fd_or_id) calls AsyncConnection::tick(uint64_t id), the ceph/master removes the assert,but this issue exits.

@shangfufei
Copy link
Author

the core infor is this:
(gdb) bt
#0 0x00007feb76d3dfcb in raise () from /lib64/libpthread.so.0
#1 0x00007feb7888513b in reraise_fatal (signum=6) at global/signal_handler.cc:135
#2 handle_fatal_signal (signum=6) at global/signal_handler.cc:209
#3
#4 0x00007feb747715f7 in raise () from /lib64/libc.so.6
#5 0x00007feb74772ce8 in abort () from /lib64/libc.so.6
#6 0x00007feb78997f6b in icfs::__icfs_assert_fail (assertion=assertion@entry=0x7feb78c3be37 "last_tick_id == id",
file=file@entry=0x7feb78c3b888 "msg/async/AsyncConnection.cc", line=line@entry=2642,
func=func@entry=0x7feb78c3c4e0 <AsyncConnection::tick(unsigned long)::PRETTY_FUNCTION> "void AsyncConnection::tick(uint64_t)")
at common/assert.cc:80
#7 0x00007feb78ae5b7e in AsyncConnection::tick (this=0x7fec37d08000, id=18446744071562068035) at msg/async/AsyncConnection.cc:2642
#8 0x00007feb78a79581 in EventCenter::process_time_events (this=this@entry=0x7feb828bd890) at msg/async/Event.cc:350
#9 0x00007feb78a7ac10 in EventCenter::process_events (this=this@entry=0x7feb828bd890, timeout_microseconds=,
timeout_microseconds@entry=30000000) at msg/async/Event.cc:423
#10 0x00007feb78ac93ba in NetworkStack::__lambda0::operator() (__closure=0x7feb828800f0) at msg/async/Stack.cc:76
#11 0x00007feb750ca220 in ?? () from /lib64/libstdc++.so.6
#12 0x00007feb76d36dc5 in start_thread () from /lib64/libpthread.so.0
#13 0x00007feb7483221d in clone () from /lib64/libc.so.6
(gdb) f 8
#8 0x00007feb78a79581 in EventCenter::process_time_events (this=this@entry=0x7feb828bd890) at msg/async/Event.cc:350
350 cb->do_request(id);
(gdb) p id
$1 = 2147483715
(gdb) f 7
#7 0x00007feb78ae5b7e in AsyncConnection::tick (this=0x7fec37d08000, id=18446744071562068035) at msg/async/AsyncConnection.cc:2642
2642 assert(last_tick_id == id);
(gdb) p id
$2 = 18446744071562068035
(gdb) p last_tick_id
$3 = 2147483715
(gdb)

@yuyuyu101
Copy link
Member

oh, right. but we should modify the EventCallback interface instead of this fix

@shangfufei
Copy link
Author

i think it will modify many files.

@yuyuyu101
Copy link
Member

not so many. it's desired

@shangfufei
Copy link
Author

ok, i will fix this bug as your method

@shangfufei
Copy link
Author

please review @yuyuyu101

@tchaikov
Copy link
Contributor

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes

Signed-off-by: shangfufei <shangfufei@inspur.com>
@shangfufei
Copy link
Author

ok , changed @tchaikov

Copy link

@amitkumar50 amitkumar50 left a comment

Choose a reason for hiding this comment

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

LGTM

@yuriw
Copy link
Contributor

yuriw commented Oct 11, 2017

@yuriw yuriw merged commit 5cfe11e into ceph:master Oct 12, 2017
@shangfufei shangfufei deleted the wip-async-fix branch March 10, 2018 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants