bcftools query --list-samples --samples-file can cause segfault

[freeseek]

When reading a sample file to list the samples in a VCF, the vcfquery.c code calls function:

static void list_columns(args_t *args)
{
    void *has_sample = NULL;
    if ( args->sample_list )
    {
        has_sample = khash_str2int_init();
        int i, nsmpl;
        char **smpl = hts_readlist(args->sample_list, args->sample_is_file, &nsmpl);
        for (i=0; i<nsmpl; i++) khash_str2int_inc(has_sample, smpl[i]);
        free(smpl);
    }

   ...
}

There is a missing check for the sample file to have correctly open. This line is missing after calling function hts_readlist():

if ( !smpls ) error("Could not parse %s\n", args->sample_list);

which can cause the code to segfault if the file is missing.

Slightly related, it seems like bcftools query and bcftools convert have support for exclusion with "^" prefix, this is not fully supported. As an example, this is code in smpl_ilist.c:

char **list = hts_readlist(negate?sample_list+1:sample_list, is_file, &nlist);
if ( !list ) error("Could not parse %s\n", sample_list);

While this is code in vcfconvert.c:

char **smpls = hts_readlist(args->sample_list, args->sample_is_file, &n);
if ( !smpls ) error("Could not parse %s\n", args->sample_list);

and this is code vcfquery.c:

char **smpls = hts_readlist(args->sample_list, args->sample_is_file, &n);
if ( !smpls ) error("Could not parse %s\n", args->sample_list);

Both snippets of code do not take the possibility of exclusion into account.

This leads to inconsistent behavior, as shown in the following example:

(echo "##fileformat=VCFv4.2"
echo "##contig=<ID=chr1>"
echo "##FORMAT=<ID=GT,Number=1,Type=String,Description=\"Genotype\">"
echo -e "#CHROM\tPOS\tID\tREF\tALT\tQUAL\tFILTER\tINFO\tFORMAT\tA\tB"
echo -e "chr1\t10000\t.\tA\tC\t.\t.\t.\tGT\t0/0\t0/1") > A.vcf
echo "A" > A.lst

$ bcftools query --format "[%SAMPLE\n]" --samples-file A.lst A.vcf
A

$ bcftools query --list-samples --samples-file A.lst A.vcf
A

$ bcftools query --format "[%SAMPLE\n]" --samples-file ^A.lst A.vcf
B

$ bcftools query --list-samples --samples-file ^A.lst A.vcf

With the last command producing no output (and potentially yielding a segfault).

Also the following behavior is slightly inconsistent: bcftools query does not have the option --force-samples as bcftools view does but the behavior is not consistent:

echo -e "A\nC" > B.lst

$ bcftools query --format "[%SAMPLE\n]" --samples-file B.lst A.vcf
Sample name mismatch: sample #2 not found in the header

$ bcftools query --list-samples --samples-file B.lst A.vcf
A

[pd3]

This is now fixed. Thanks for the issue

[freeseek]

Very much thank you for the improvement here.

There is still the main issue leading to segfault in the new vcfquery.c code:

static void list_columns(args_t *args)
{
    int negate = 0;
    int i;
    bcf_sr_t *reader = &args->files->readers[0];
    void *has_sample = NULL;
    if ( args->sample_list )
    {
        if ( args->sample_list[0]=='^' ) negate = 1;
        has_sample = khash_str2int_init();
        int i, nsmpl;
        char **smpl = hts_readlist(negate ? args->sample_list+1 : args->sample_list, args->sample_is_file, &nsmpl);
        for (i=0; i<nsmpl; i++)
        {
            if ( bcf_hdr_id2int(reader->header,BCF_DT_SAMPLE,smpl[i])<0 && !args->force_samples )
                error("Error: sample #%d not found in the header, user --force-samples to proceed anyway\n", i+1);
            khash_str2int_inc(has_sample, smpl[i]);
        }
        free(smpl);
    }
    ...
}

that if the hts_readlist() function fails, for example if the indicated file is missing, then nsmpl will not be defined and then the following loop for (i=0; i<nsmpl; i++) would use the uninitialized variable nsmpl leading to doing nothing if nsmpl is 0 and leading to a likely segfault otherwise. In general the code should always check whether hts_readlist(const char *string, int is_file, int *_n) returns NULL in which case the value of _n should not be used as it would be undefined.

By looking at all files, there are instances of not checking the return of hts_readlist() in bin.c, filter.c, mpileup.c, vcfconvert.c (4 instances), vcffilter.c, vcfquery.c, plugins/color-chrs.c (2 instances), plugins/contrast.c, plugins/fill-tags.c, plugins/GTsubset.c, plugins/mendelian.c (2 instances), plugins/parental-origin.c, plugins/scatter.c and plugins/split.c (though not sure for these two how this is handled by the code later on), plugins/trio-dnm2.c, and plugins/trio-stats.c

[pd3]

I added a few more checks for the missing cases 77aba45.

However, in most of the files you listed above the function is used to parse a string and can fail only on memory allocation errors. Strictly speaking those should be checked as well, but if memory is so low that malloc fails on cases like that, the program will soon abort anyway, so leaving those cases untouched for now.