Bug 10400 - Text::CSV_XS error-checking should be standardized
Summary: Text::CSV_XS error-checking should be standardized
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Label/patron card printing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Chris Nighswonger
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-04 14:36 UTC by Galen Charlton
Modified: 2023-07-08 15:59 UTC (History)
0 users

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Galen Charlton 2013-06-04 14:36:09 UTC
As reported by Doug Kingston on koha-devel on 2013-06-02, if version 0.97 of Text::CSV_XS is installed, it can segfault.  From Doug's email:

"The error from t/Labels.t is a segmentation fault when calling
$csv->error_input() in Labels/Label.pm.

sub _get_text_fields {
    my $format_string = shift;
    my $csv = Text::CSV_XS->new({allow_whitespace => 1});
    my $status = $csv->parse($format_string);
    my @sorted_fields = map {{ 'code' => $_, desc => $_ }}
                        map { $_ eq 'callnumber' ? 'itemcallnumber' : $_ }
# see bug 5653
                        $csv->fields();
    my $error = $csv->error_input();
    warn sprintf('Text field sort failed with this error: %s', $error) if
$error;
    return \@sorted_fields;
}"

A ticket was opened with the maintainer of Text::CSV_XS (https://rt.cpan.org/Ticket/Display.html?id=85810), and he fixed the segfault bug in version 0.98 of the module.

Nonetheless, Koha code that uses Text::CSV_XS should follow the module's documentation more closely; in particular, it looks like the correct thing to do is the check the value of the $status returned by $csv->parse() before deciding whether to invoke error_input().
Comment 1 Galen Charlton 2013-06-04 15:20:36 UTC
Also, in that snippet of code, there's no point in calling ->fields() until we know that the parsing was successful.
Comment 2 Galen Charlton 2013-06-05 14:54:28 UTC
Testing shows that if you pass an invalid field list to _get_text_fields(), the result is:

$VAR1 = [
          {
            'desc' => undef,
            'code' => undef
          }
        ];

That seems a bit untidy; I think it ought to return either undef or a ref to an empty list in that case.
Comment 3 Katrin Fischer 2023-07-08 15:59:19 UTC
(In reply to Galen Charlton from comment #0)
> As reported by Doug Kingston on koha-devel on 2013-06-02, if version 0.97 of
> Text::CSV_XS is installed, it can segfault.  From Doug's email:
> 
> "The error from t/Labels.t is a segmentation fault when calling
> $csv->error_input() in Labels/Label.pm.
> 
> sub _get_text_fields {
>     my $format_string = shift;
>     my $csv = Text::CSV_XS->new({allow_whitespace => 1});
>     my $status = $csv->parse($format_string);
>     my @sorted_fields = map {{ 'code' => $_, desc => $_ }}
>                         map { $_ eq 'callnumber' ? 'itemcallnumber' : $_ }
> # see bug 5653
>                         $csv->fields();
>     my $error = $csv->error_input();
>     warn sprintf('Text field sort failed with this error: %s', $error) if
> $error;
>     return \@sorted_fields;
> }"
> 
> A ticket was opened with the maintainer of Text::CSV_XS
> (https://rt.cpan.org/Ticket/Display.html?id=85810), and he fixed the
> segfault bug in version 0.98 of the module.

koha-testing-docker shows the version installed as:

Text::CSV_XS (0.32 ) 	1.45 

So I guess we could assume that most installations have this fix by now.

> 
> Nonetheless, Koha code that uses Text::CSV_XS should follow the module's
> documentation more closely; in particular, it looks like the correct thing
> to do is the check the value of the $status returned by $csv->parse() before
> deciding whether to invoke error_input().

It looks like we do the status check on patron import:

        my $status  = $self->text_csv->parse($borrowerline);
        my @columns = $self->text_csv->fields();
        if ( !$status ) {
            push @missing_criticals, { badparse => 1, line => $line_number, lineraw => decode_utf8($borrowerline) };
        }

But there has not been any change in Labels:

sub _get_text_fields {
    my $format_string = shift;
    my $csv = Text::CSV_XS->new({allow_whitespace => 1});
    my $status = $csv->parse($format_string);
    my @sorted_fields = map {{ 'code' => $_, desc => $_ }} 
                        map { $_ && $_ eq 'callnumber' ? 'itemcallnumber' : $_ } # see bug 5653
                        $csv->fields();
    my $error = $csv->error_input();
    warn sprintf('Text field sort failed with this error: %s', $error) if $error;
    return \@sorted_fields;