MySQL Bugs: #44332: my_xml_scan reads behind the end of buffer

Bug #44332	my_xml_scan reads behind the end of buffer
Submitted:	17 Apr 2009 0:26	Modified:	28 Jan 2011 12:14
Reporter:	Vladislav Vaintroub	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: XML functions	Severity:	S2 (Serious)
Version:	6.0, 5.1, 5.6.99 bzr	OS:	Any
Assigned to:	Alexander Barkov	CPU Architecture:	Any

Description:
Found when testing mysqld on Windows with full pageheap enabled..

in my_xml_scan, in following fragment, bcmp function is called but there is no check that it can 9 bytes can be read
  else if (!bcmp(p->cur, "<![CDATA[",9))

How to repeat:
Code inspection

To see a crash "live" on Windows, install Debugging tools for Windows, enable full heap checking for mysqld with 

Gflags.exe -p /enable  mysqld.exe  /full /unaligned

and start the test suite. mysqld.exe will crash on the line above

Suggested fix:
Check end-of-buffer condition, e.g 

 else if ((p->end - p->cur > 8) && !bcmp(p->cur, "<![CDATA[",9))

The bug is similar to Bug#2399

Bug exists in 5.1 too. Now it uses memcmp and does not check length as well. Looks like bug #2399 was fixed for memcmp(p->cur,"<!--",4) only and not for next string (!memcmp(p->cur, "<![CDATA[",9))

Suggested fix:

=== modified file 'strings/xml.c'
--- strings/xml.c       2010-07-02 18:30:47 +0000
+++ strings/xml.c       2010-08-31 18:51:40 +0000
@@ -132,7 +132,7 @@
     a->end=p->cur;
     lex=MY_XML_COMMENT;
   }
-  else if (!memcmp(p->cur, "<![CDATA[",9))
+  else if ((p->end - p->cur > 8) && !memcmp(p->cur, "<![CDATA[",9))
   {
     p->cur+= 9;
     for (; p->cur < p->end - 2 ; p->cur++)

Here's a testcase that shows valgrind warnings on 5.6.2:
select updatexml(convert('<' using utf8),'1','1');

Conditional jump or move depends on uninitialised value(s) 
at: bcmp (mc_replace_strmem.c:541)                         
by: my_xml_scan (xml.c:135)                                
by: my_xml_parse (xml.c:278)                               
by: Item_xml_str_func::parse_xml (item_xmlfunc.cc:2778)    
by: Item_func_xml_update::val_str (item_xmlfunc.cc:2816)   
by: Item::send (item.cc:5931)                              
by: Protocol::send_result_set_row (protocol.cc:848)        
by: select_send::send_data (sql_class.cc:1866)             
by: JOIN::exec (sql_select.cc:2794)                        
by: mysql_select (sql_select.cc:3554)                      
by: handle_select (sql_select.cc:323)                      
by: execute_sqlcom_select (sql_parse.cc:4513)              
by: mysql_execute_command (sql_parse.cc:2096)              
by: mysql_parse (sql_parse.cc:5550)                        
by: dispatch_command (sql_parse.cc:1078)                   
by: do_command (sql_parse.cc:815)                          
by: do_handle_one_connection (sql_connect.cc:748)          
by: handle_one_connection (sql_connect.cc:684)             
by: start_thread (pthread_create.c:301)

Another problem in the same code, read behind the end of the string
when processing incomplete XML comment:

mysql> select updatexml(convert('<!--' using utf8),'1','1');

==19617== Conditional jump or move depends on uninitialised value(s)
==19617==    at 0x4A07AF3: bcmp (mc_replace_strmem.c:646)
==19617==    by 0xB9521C: my_xml_scan (xml.c:137)
==19617==    by 0xB95A90: my_xml_parse (xml.c:285)
==19617==    by 0x678983: Item_xml_str_func::parse_xml(String*, String*) (item_xmlfunc.cc:2770)

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/128988

3558 Alexander Barkov	2011-01-17
      Bug#44332 my_xml_scan reads behind the end of buffer
      
      Problem: the scanner function tested for strings "<![CDATA[" and
      "<--" without checking input string boundaries, which led to valgrind's
      "Conditional jump or move depends on uninitialised value(s)" error.
      
      Fix: Adding boundary checking.
      
        @ mysql-test/r/xml.result
        @ mysql-test/t/xml.test
        Adding test
      
        @ strings/xml.c
        Adding a helper function my_xml_parser_prefix_cmp(),
        with input string boundary check.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/129017

3559 Alexander Barkov	2011-01-18
      Bug#44332 my_xml_scan reads behind the end of buffer
      
      Problem: the scanner function tested for strings "<![CDATA[" and
      "-->" without checking input string boundaries, which led to valgrind's
      "Conditional jump or move depends on uninitialised value(s)" error.
      
      Fix: Adding boundary checking.
      
        @ mysql-test/r/xml.result
        @ mysql-test/t/xml.test
        Adding test
      
        @ strings/xml.c
        Adding a helper function my_xml_parser_prefix_cmp(),
        with input string boundary check.

Pushed into mysql-5.1 5.1.56 (revid:alexander.barkov@oracle.com-20110118063841-4hryslwcfpyrp606) (version source revid:alexander.barkov@oracle.com-20110118063841-4hryslwcfpyrp606) (merge vers: 5.1.56) (pib:24)

Pushed into mysql-5.5 5.5.10 (revid:alexander.barkov@oracle.com-20110118065003-65h5ws819pr5du7d) (version source revid:alexander.barkov@oracle.com-20110118065003-65h5ws819pr5du7d) (merge vers: 5.5.10) (pib:24)

Pushed into mysql-trunk 5.6.2 (revid:alexander.barkov@oracle.com-20110118065345-xj7b0r11c2yj0v3m) (version source revid:alexander.barkov@oracle.com-20110118065345-xj7b0r11c2yj0v3m) (merge vers: 5.6.2) (pib:24)

Documented bugfix in the 5.1.56, 5.5.10, and 5.6.2 changelogs, as follows:

        When using ExtractValue() or UpdateXML(), if the XML to be read
        contained an incomplete XML comment, MySQL read beyond the end
        of the string when processing, leading to a crash of the server.

Closed.

Already documented as noted above; set back to Closed.