Bug 1087104

There are those that would argue that "-1" could be used to imply the largest value allowed. The libvirt code reads input from virsh generally as an unsigned long long value and then makes decisions based on "expectations" of how the caller wishes to interpret the data.

For "offset" the eventual lseek() allows a -1 value to mean the end of the file, although other negative values are interpreted as their unsigned long long values which generally results in an error since files are generally not that large (yet).  Whether it's realistic to expect copying from the end of the file means anything is left to the interpretation of the user and the implementation of the OS. See the lstat man page for details on -1 for the offset argument.

For "length" a -1 could/should be interpretted as the "whole" file. In your example all 197120 bytes. Other negative values are less obvious, but would be converted to their unsigned long long equivalent and used. In your case -1000 would be more than enough to copy the whole file.

For a short time during May 2014 it seems the use of -1 would have been prohibited due to other changes (listed below) in the underlying code called by the virsh routines (in particular vshCommandOptULongLong()). The following is a bit of history going backwards through time...

For vol-{upload|download} in particular, a change made as a result of the patches restored the ability to use -1 as a pseudonym for everything:

http://www.redhat.com/archives/libvir-list/2014-June/msg00184.html

Those changes were based on a pass where the vcpuinfo parameter to allow/use -1:

http://www.redhat.com/archives/libvir-list/2014-May/msg01013.html

was questioned and led to a list of various virsh commands that may or may not want to allow -1:

http://www.redhat.com/archives/libvir-list/2014-May/msg01029.html

The set of changes was based upon a new API that was developed to handle those -1 special cases based on the following patches:

https://www.redhat.com/archives/libvir-list/2014-May/msg00000.html

which were based on the rejection of:

http://www.redhat.com/archives/libvir-list/2014-April/msg01132.html

The early May patches would have caused -1 to be rejected as an invalid value, while the later May patches restored the former functionality based on the auditing done of the callers of the API's to convert input values.

I have reopened (with any luck) the discussion surrounding vol-{upload|download} with the upstream community and will see what the thoughts there are.  I personally think an 'offset' of -1 is useless, although a negative value for length does have value to mean everything.

The man page will be adjusted accordingly based on the results of the renewed discussion.  If that doesn't work - I'll send patches which will most certainly cause someone to think about it again

Patch sent upstream:

http://www.redhat.com/archives/libvir-list/2014-July/msg00865.html


Code pushed:

commit 570d0f6387038a6d7651b22fd555de603f07ab3b
Author: John Ferlan <jferlan>
Date:   Tue Jul 15 08:26:28 2014 -0400

    virsh vol-upload/download disallow negative offset
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1087104
    
    Commit id 'c6212539' explicitly allowed a negative value to be used for
    offset and length as a shorthand for the largest value after commit id
    'f18c02ec' modified virStrToLong_ui() to essentially disallow a negative
    value.
    
    However, allowing a negative value for offset ONLY worked if the negative
    value was -1 since the eventual lseek() does allow a -1 to mean the end
    of the file.  Providing other negative values resulted in errors such as:
    
    $ virsh vol-download --pool default qcow3-vol2 /home/vm-images/raw \
      --offset -2 --length -1000
    error: cannot download from volume qcow3-vol2
    error: Unable to seek /home/vm-images/qcow3-vol2 to 18446744073709551614: In
    
    $
    
    Thus, it seems unreasonable to expect or allow a negative value for offset
    since the only benefit is to lseek() to the end of the file and then only
    take advantage of how the OS would handle such a seek. For the purposes of
    upload or download of volume data, that seems to be a no-op.  Therefore,
    disallow a negative value for offset.
    
    Additionally, modify the man page for vol-upload and vol-download to provide
    more details regarding the valid values for both offset and length.



git describe: v1.2.6-184-g570d0f6

According to the title and content of the patch in comment 3 ,
and the description of vol-upload / vol-download about --offset ,--length option in the man page :
......
--offset is the position in the storage volume at which to start writing the data. The value must be 0 or larger
--length is an upper bound of the amount of data to be uploaded. A negative value is interpreted as an unsigned long long value to essentially include everything from the offset to the end of the volume. 
......
 
verifying it as following steps , it seems that it doesn't work well for vol-upload :

steps:
1> 
try to do vol-upload with a negative value of --offset as -1 and --length as -1 , upload successfully

# virsh vol-upload --pool default --vol vol1.xml uploadfile --offset -1 --length -1

2>
try to do vol-upload with a negative value of --offset as -1 and --length as -10000 , upload successfully
# virsh vol-upload --pool default --vol vol1.xml uploadfile --offset -1 --length -10000

3>
try to do vol-upload with a negative value of --offset as -2 and --length as -1 , fail to upload , give error message .

# virsh vol-upload --pool default --vol vol1.xml uploadfile --offset -2 --length -1
error: cannot upload to volume vol1.xml
error: End of file while reading data: Input/output error
error: Failed to reconnect to the hypervisor

4>
try to do vol-upload with a negative value of --offset as -1000 and --length as -1000 , fail to upload , give error message .

# virsh vol-upload --pool default --vol vol1.xml uploadfile --offset -1000 --length -1000
error: cannot upload to volume vol1.xml
error: End of file while reading data: Input/output error
error: Failed to reconnect to the hypervisor

5>try to do vol-download with a negative value of --offset  and --length , unable to parse offset value

# virsh vol-download --pool default --vol vol1.xml downloadfile --offset -2 --length -1
error: Unable to parse offset value

# virsh vol-download --pool default --vol vol1.xml downloadfile --offset -1 --length -1
error: Unable to parse offset value


As above ,for command vol-upload in the step 1 and step 2 , if the negative value of offset is -1 , it will be wrapped the largest value then upload successfully.
in the step 3 and step 4 , using another negative value which is not -1 , fail to upload and it  will give a confused error message.
should it use the same error handling as well as vol-download . 
thanks.

yes - although it's been 5 months since I made the change and I have no recollection how I forgot the -upload check as well even though I mentioned it in the commit (sigh!)... Must've been the myopia and short term memory loss.

I see Shanzhi Yu has submitted a patch to the upstream list - I will return this bz back to POST once I push that and repost to the downstream list

verify version:

libvirt-1.2.8-11.el7.x86_64
qemu-kvm-rhev-2.1.2-17.el7.x86_64
kernel-3.10.0-217.el7.x86_64

verify steps:

1.create two volumes in the default pool

# cat default-volume.xml 
<volume>
  <name>qcow2-vol</name>
  <source>
  </source>
  <capacity unit='bytes'>1024000000</capacity>
  <allocation unit='bytes'>204000</allocation>
  <target>
    <format type='qcow2'/>
   <features>
    <lazy_refcounts/>
    </features>
  </target>
</volume>

 
# virsh vol-create default default-volume.xml 
Vol qcow2-vol created from default-volume.xml


# cat raw-vol.xml 
<volume>
<name>raw-vol</name>
  <source>
  </source>
   <capacity unit='bytes'>589934592</capacity>
  <allocation unit='bytes'>56063232</allocation>
  <target>
    <format type='raw'/>
  </target>
</volume>

# virsh vol-create default raw-vol.xml 
Vol raw-vol created from raw-vol.xml

check volumes in default pool 

# virsh vol-list default
 Name                 Path                                    
------------------------------------------------------------------------------
 qcow2-vol            /var/lib/libvirt/images/qcow2-vol       
 raw-vol              /var/lib/libvirt/images/raw-vol         


2. verify vol-upload/download disallow negative offset

2.1  try to download qcow2-vol to raw by passing negative integer to offset and length 

# virsh vol-download --pool default qcow2-vol /var/lib/libvirt/images/raw-vol --offset -1 --length -1000
error: Unable to parse offset value

# virsh vol-download --pool default qcow2-vol  /var/lib/libvirt/images/raw-vol --offset -2 --length -10
error: Unable to parse offset value

2.2  try to upload qcow2 to raw by passing negative integer to offset and length 

# virsh vol-upload --pool default qcow2-vol /var/lib/libvirt/images/raw-vol --offset -1 --length -1000
error: Unable to parse offset value

# virsh vol-upload --pool default qcow2-vol /var/lib/libvirt/images/raw-vol --offset -2 --length -100
error: Unable to parse offset value 


3.verify using vol-upload/download with an offset that was "too large" 

3.1  try to download qcow2-vol to raw by passing an offset with too large value 
 
# virsh vol-download --pool default qcow2-vol /var/lib/libvirt/images/raw-vol --offset 18446744073709551614 --length 18446744073709551614
error: cannot download to volume qcow2-vol
error: Unable to seek /var/lib/libvirt/images/qcow2-vol to 18446744073709551614: Invalid argument

NOTE : if set an offset that was 18446744073709551615 ,it will download successfully.
# virsh vol-download --pool default qcow2-vol /var/lib/libvirt/images/raw-vol --offset 18446744073709551615 --length 18446744073709551615

# virsh vol-download --pool default qcow2-vol /var/lib/libvirt/images/raw-vol --offset 18446744073709551616 --length 18446744073709551616
error: Unable to parse offset value

3.2   try to upload qcow2 to raw by passing an offset with too large value 

# virsh vol-upload --pool default qcow2-vol /var/lib/libvirt/images/raw-vol --offset 18446744073709551614 --length 18446744073709551615
error: cannot upload to volume qcow2-vol
error: Unable to seek /var/lib/libvirt/images/qcow2-vol to 18446744073709551614: Invalid argument

NOTE : if set an offset that was 18446744073709551615 ,it will upload successfully.
# virsh vol-upload --pool default qcow2-vol /var/lib/libvirt/images/raw-vol --offset 18446744073709551615 --length 18446744073709551615

# virsh vol-upload --pool default qcow2-vol /var/lib/libvirt/images/raw-vol --offset 18446744073709551616 --length 18446744073709551616
error: Unable to parse offset value


Is it OK to verify this bug and the CVE issue according to all above steps .

And another issue in steps 3.1 and 3.2  when i try to set a too large offset .It will upload / download successfully if set an offset which was 18446744073709551615 .
but the value 18446744073709551614 (or a little less than 18446744073709551614  or 18446744073709551616) was invalid  and will give the error message. If that is expected result or may be another question.
Thanks a lot .

From the aspect of whether the bug described is fixed - I would say, sure this is verified (e.g. -1 as offset results in failures).

From the aspect of whether the *1614, *1615, and *1616 work - I think perhaps a new bug report should be created and tracked separately, but be sure it gets the rhel-7.2.0 flag (not 7.1.0).  The *1615 value is also known as UINT64_MAX and it's worth the time to chase into the code to see if that magic value has special meaning...  It might end up being nothing but the *equivalent* of passing -1 to *seek to indicate "the whole file" or "the end of the file".

Thanks for your response . 
According to comment 9 and comment 10 , move this bug to verified .
A new bug to trace "too large" value issue .
https://bugzilla.redhat.com/show_bug.cgi?id=1177219

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHSA-2015-0323.html