Bug 1167069

Summary: Segmentation fault when calling libvirt_network_set_active
Product: [Community] Virtualization Tools Reporter: James <jrhodes>
Component: libvirt-phpAssignee: Michal Privoznik <mprivozn>
Status: CLOSED NEXTRELEASE QA Contact:
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: unspecifiedCC: crobinso, mprivozn
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: libvirt-php-0.5.2-39-gc725824 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-09-21 12:22:40 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description James 2014-11-23 07:14:07 UTC
Description of problem:
Calling libvirt_network_set_active causes a segmentation fault when the second parameter is passed.

Version-Release number of selected component (if applicable):
0.4.8

How reproducible:
Every time.

Steps to Reproduce:
<?php

$conn = libvirt_connect('connection_uri', false);

$network = libvirt_network_get($conn, 'network_name_here');

// This will return an error stating that it expected 2 parameters (the
// documentation states it only accepts 1 parameter, but that doesn't
// make much sense for this function call).
libvirt_network_set_active($network);

// Both of these calls will cause a segmentation fault.
libvirt_network_set_active($network, true);
libvirt_network_set_active($network, 1);

?>

Actual results:
Segmentation fault when calling libvirt_network_set_active.

Expected results:
Network active status should be set as intended.

Additional info:

Comment 2 James 2014-11-23 08:19:33 UTC
I'm pretty sure this is a bug in PHP.  After modifying the libvirt to do some debugging, expanding constants and adding some debugging printf's, I ended up with this:

--------------------------------------------
PHP_FUNCTION(libvirt_network_set_active)                                                                                               
{                                                                                                                                      
        php_libvirt_network *network = NULL;                                                                                           
        zval *znetwork;
        unsigned int act = 0;                                                                                                          
        
        reset_error(TSRMLS_C);                                                                                                         
        if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "rl", &znetwork, &act) == FAILURE) {                                      
                set_error("Invalid arguments" TSRMLS_CC);                                                                              
                RETURN_FALSE;
        }                                                                                                                              
        
        if (znetwork == NULL) {
                printf("znetwork is NULL.\n");                                                                                         
        } else {                                                                                                                       
                printf("znetwork is not NULL.\n");                                                                                     
        }
        
        zend_uchar tt;
        tt = znetwork->type;                                                                                                           
        
        printf("Type of resource is %d\n", tt);
        
        printf("About to call ZEND_FETCH_RESOURCE...\n");                                                                              
        network = (php_libvirt_network*)zend_fetch_resource(&znetwork TSRMLS_CC, -1, PHP_LIBVIRT_NETWORK_RES_NAME, NULL, 1, le_libvirt_network);
        if (!network) {                                                                                                                
                RETURN_FALSE;
        }
        if (network == NULL || network->network == NULL) {
                RETURN_FALSE;
        }

        if ((act != 0) && (act != 1)) {
                set_error("Invalid network activity state" TSRMLS_CC);                                                                 
                RETURN_FALSE;
        }
--------------------------------------------

When running under gdb, I get a segmentation fault on the "tt = znetwork->type;" line, indicating that the value of znetwork isn't a valid pointer.  It writes out "znetwork is not NULL." before this, indicating that PHP is giving back an invalid pointer.

Interestingly enough, I can fix the issue by changing the parameter declaration to:

--------------------------------------------
PHP_FUNCTION(libvirt_network_set_active)                                                                                               
{                                                                                                                                      
        php_libvirt_network *network = NULL;                                                                                           
        zval *znetwork;
        zend_bool act = 0; 
        
        reset_error(TSRMLS_C);                                                                                                         
        if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "rb", &znetwork, &act) == FAILURE) {                                      
                set_error("Invalid arguments" TSRMLS_CC);                                                                              
                RETURN_FALSE;
        } 
--------------------------------------------

This doesn't crash, and znetwork has a valid value, all because I changed the second parameter from long to boolean ("rl" to "rb").  I verified with other functions that "rl" looks like it's being used correctly (storing in the correct type), so I'm not sure why PHP is returning an invalid pointer here, and why it only seems to apply to this function (no other networking functions are affected).

Comment 3 James 2014-11-23 08:46:15 UTC
Filed a bug in PHP upstream in the event that this is a bug in the PHP parameter parsing: https://bugs.php.net/bug.php?id=68484

Comment 4 James 2014-11-23 08:49:07 UTC
Here's a patch for the "change parameter type to bool" fix, although it'd be much nicer to discover why "rl" causes this issue in the first place:

--- libvirt.c.old       2014-11-23 19:48:03.350647661 +1100
+++ libvirt.c   2014-11-23 19:48:10.934806105 +1100
@@ -7564,9 +7564,9 @@
 {
        php_libvirt_network *network;
        zval *znetwork;
-       int act = 0;
+       zend_bool act = 0;
 
-       GET_NETWORK_FROM_ARGS("rl",&znetwork,&act);
+       GET_NETWORK_FROM_ARGS("rb",&znetwork,&act);
 
        if ((act != 0) && (act != 1)) {
                set_error("Invalid network activity state" TSRMLS_CC);

Comment 5 James 2014-11-23 11:19:58 UTC
According to https://bugs.php.net/bug.php?id=68484 it's because the size of int and long can differ.  Because the arguments are pushed in reverse order, the size difference causes corruption of the network parameter.

The following functions are presumably susceptible to the same issue, because they use an int or unsigned int variable for accepting one or more long parameters:

libvirt_domain_get_screenshot_api (screen)
libvirt_domain_get_screenshot (scancode)
libvirt_domain_send_pointer_event (pos_x, pos_y, clicked)
libvirt_domain_change_vcpus (numCpus only)
libvirt_domain_new (memMB,maxmemMB,vcpus)
libvirt_storagevolume_delete (flags)
libvirt_domain_snapshot_delete (flags)

Comment 6 Cole Robinson 2016-03-23 20:13:28 UTC
Thanks for the detailed info, sorry this never received a response. If you're still interested, the best way to get this fixed is to send a patch to libvir-list and CC mprivozn , but I'm CCing him here, maybe he'll get to it first :)

Comment 7 Michal Privoznik 2016-09-21 12:22:40 UTC
I believe this is fixed by these two commits (and possibly others too)

commit cc8190ba60f911e04eb10a0e28dc3c9123e15bae
Author:     Remi Collet <fedora>
AuthorDate: Wed Apr 13 12:13:08 2016 -0400
Commit:     Michal Privoznik <mprivozn>
CommitDate: Thu Apr 14 18:24:36 2016 +0200

    add missing arginfo
    
    Signed-off-by: Michal Privoznik <mprivozn>


commit 9f355ebed7a217669269c1a4016329f7411e356b
Author:     Michal Novotny <minovotn>
AuthorDate: Fri Jul 19 13:20:46 2013 +0200
Commit:     Michal Novotny <minovotn>
CommitDate: Fri Jul 19 13:20:46 2013 +0200

    Fix libvirt_network_set_active