Alfranio Correia wrote:
> Hi Jasonh,
>
> Sorry for the mistake.
>
NP
> You are reading the event and not writing.
> Patch approved.
>
Thanks! and I have committed a new patch, which modified more usages of
strcpy in semisync to strncpy that I think might suffer this, it would
be appreciated if you could have a look!
> Cheers.
>
> He Zhenxing wrote:
> > Alfranio Correia wrote:
> >> Hi Jasonh,
> >>
> >> I did not understand the patch.
> >> See some questions in-line.
> >>
> >
> > The purpose of this patch is to make the semisync code safer from
> > possible buffer overflow attacks.
> >
> >> Let's talk in the IRC.
> >>
> >> Cheers.
> >>
> >> He Zhenxing wrote:
> >>> #At file:///media/sdb2/hezx/work/mysql/bzr/b52748/trunk-bugfixing/ based
> on revid:jon.hauglid@stripped
> >>>
> >>> 3057 He Zhenxing 2010-05-24
> >>> BUG#52748 Semi-Sync ACK packet isn't check for length
> >>>
> >>> Check the length and use strncpy to make the code safer.
> >>>
> >>> modified:
> >>> plugin/semisync/semisync_master.cc
> >>> === modified file 'plugin/semisync/semisync_master.cc'
> >>> --- a/plugin/semisync/semisync_master.cc 2010-03-11 02:22:18 +0000
> >>> +++ b/plugin/semisync/semisync_master.cc 2010-05-24 02:57:37 +0000
> >>> @@ -1048,6 +1048,7 @@ int ReplSemiSyncMaster::readSlaveReply(N
> >>> const unsigned char *packet;
> >>> char log_file_name[FN_REFLEN];
> >>> my_off_t log_file_pos;
> >>> + ulong log_file_len = 0;
> >>> ulong packet_len;
> >>> int result = -1;
> >>>
> >>> @@ -1123,7 +1124,13 @@ int ReplSemiSyncMaster::readSlaveReply(N
> >>> }
> >>>
> >>> log_file_pos = uint8korr(packet + REPLY_BINLOG_POS_OFFSET);
> >>> - strcpy(log_file_name, (const char*)packet +
> REPLY_BINLOG_NAME_OFFSET);
> >>> + log_file_len = packet_len - REPLY_BINLOG_NAME_OFFSET;
> >> Is this the reserved space in the packet for the log file name?
> >>
> >
> > REPLY_BINLOG_NAME_OFFSET is the start position for the log file name in
> > the reply packet, the log file name will span to the end of the packet,
> > so the length of the log file will be:
> > packet_len - REPLY_BINLOG_NAME_OFFSET
> >
> >>> + if (log_file_len > FN_REFLEN)
> >>> + {
> >>> + sql_print_error("Read semi-sync reply binlog file length too
> large");
> >>> + goto l_end;
> >>> + }
> >> I think the "if" should be:
> >>
> >> if (FN_REFLEN > log_file_len)
> >>
> >
> > No, here the code checks the sanity of the reply packet, if the length
> > of log file in the reply packet is longer than FN_REFLEN, then we
> > considered the reply packet is forged or corrupted, and will be
> > rejected.
> >
> >>> + strncpy(log_file_name, (const char*)packet +
> REPLY_BINLOG_NAME_OFFSET, log_file_len);
> >>>
> >>> if (trc_level & kTraceDetail)
> >>> sql_print_information("%s: Got reply (%s, %lu)",
> >>>
> >
> >
>
