Re: Bug in od with "-j" option?

[Jim Meyering]

Paul GHALEB <address@hidden> wrote:
> IOW, when one tries to skip exactly the size of the file, od acts as
> if it ignored the "-j size" option.
>
> I looked at the source code and found that the problem is most likely
> in src/od.c:skip() at line 1043:
>             if ((uintmax_t) file_stats.st_size <= n_skip)
> which should be:
>             if ((uintmax_t) file_stats.st_size <  n_skip)

Thank you for the report and patch.
That bug was present even on the trunk.
I've just applied your patch, adjusted the comment,
added tests and "NEWS", and pushed the result:

  http://git.sv.gnu.org/gitweb/?p=coreutils.git

diff --git a/ChangeLog b/ChangeLog
index 1cd7390..df1718e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2007-08-14  Jim Meyering  <address@hidden>
+
+       od: fix a bug that arises when skipping exact length of file
+       * NEWS: Document the bug fix.
+       * src/od.c (skip): Call fseek even when n_skip is exactly the
+       same as the length of the current file.  Otherwise, the next
+       iteration would use unadjusted input stream pointer, thus ignoring
+       the desired "skip".  Report and patch by Paul GHALEB.
+
 2007-08-10  Paul Eggert  <address@hidden>

        Accommodate more xstrtol changes.
diff --git a/NEWS b/NEWS
index 83d06b7..13d3402 100644
--- a/NEWS
+++ b/NEWS
@@ -98,6 +98,11 @@ GNU coreutils NEWS                                    -*- 
outline -*-
   ln=target attribute) would mistakenly output the string "target"
   before the name of each symlink.  [introduced in coreutils-6.0]

+  "od -j L FILE" had a bug: when the number of bytes to skip, L, is exactly
+  the same as the length of FILE, od would skip *no* bytes.  When the number
+  of bytes to skip is exactly the sum of the lengths of the first N files,
+  od would skip only the first N-1 files. [introduced in textutils-2.0.9]
+
   seq no longer mishandles obvious cases like "seq 0 0.000001 0.000003",
   so workarounds like "seq 0 0.000001 0.0000031" are no longer needed.

diff --git a/THANKS b/THANKS
index ae5269f..cdddec4 100644
--- a/THANKS
+++ b/THANKS
@@ -397,6 +397,7 @@ Oskar Liljeblad                     address@hidden
 Pádraig Brady                       address@hidden
 Patrick Mauritz                     address@hidden
 Paul Eggert                         address@hidden
+Paul Ghaleb                         address@hidden
 Paul Jarc                           address@hidden
 Paul Nevai                          address@hidden
 Paul Sauer                          address@hidden
diff --git a/src/od.c b/src/od.c
index 1e77f92..0abce59 100644
--- a/src/od.c
+++ b/src/od.c
@@ -1034,13 +1034,12 @@ skip (uintmax_t n_skip)
        {
          /* The st_size field is valid only for regular files
             (and for symbolic links, which cannot occur here).
-            If the number of bytes left to skip is at least
-            as large as the size of the current file, we can
-            decrement n_skip and go on to the next file.  */
-
+            If the number of bytes left to skip is larger than
+            the size of the current file, we can decrement
+            n_skip and go on to the next file.  */
          if (S_ISREG (file_stats.st_mode) && 0 <= file_stats.st_size)
            {
-             if ((uintmax_t) file_stats.st_size <= n_skip)
+             if ((uintmax_t) file_stats.st_size < n_skip)
                n_skip -= file_stats.st_size;
              else
                {
--
1.5.3.rc4.67.gf9286


>From 459afe0f9859fdf0e9c1a3505425c8bd345b72e3 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 14 Aug 2007 10:17:52 +0200
Subject: [PATCH] Add tests for the just-fixed "od -j N FILE" bug.
 * tests/misc/od: New file, test for the above.
 * tests/misc/Makefile.am (TESTS): Add od.


Signed-off-by: Jim Meyering <address@hidden>
---
 ChangeLog              |    3 ++
 tests/misc/Makefile.am |    1 +
 tests/misc/od          |   74 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 0 deletions(-)
 create mode 100755 tests/misc/od

diff --git a/ChangeLog b/ChangeLog
index df1718e..aa81a6c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -7,6 +7,9 @@
        iteration would use unadjusted input stream pointer, thus ignoring
        the desired "skip".  Report and patch by Paul GHALEB.

+       * tests/misc/od: New file, test for the above.
+       * tests/misc/Makefile.am (TESTS): Add od.
+
 2007-08-10  Paul Eggert  <address@hidden>

        Accommodate more xstrtol changes.
diff --git a/tests/misc/Makefile.am b/tests/misc/Makefile.am
index 8cf03f2..7206b39 100644
--- a/tests/misc/Makefile.am
+++ b/tests/misc/Makefile.am
@@ -42,6 +42,7 @@ TESTS_ENVIRONMENT = \
 # will execute the test script rather than the standard utility.

 TESTS = \
+  od \
   xstrtol \
   arch \
   pr \
diff --git a/tests/misc/od b/tests/misc/od
new file mode 100755
index 0000000..1cbbcdb
--- /dev/null
+++ b/tests/misc/od
@@ -0,0 +1,74 @@
+#!/bin/sh
+# -*- perl -*-
+# Exercise od
+
+# Copyright (C) 2006, 2007 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+: ${PERL=perl}
+: ${srcdir=.}
+
+pwd=`pwd`
+t0=`echo "$0"|sed 's,.*/,,'`.tmp; tmp=$t0/$$
+trap 'status=$?; cd "$pwd" && chmod -R u+rwx $t0 && rm -rf $t0 && exit 
$status' 0
+trap '(exit $?); exit $?' 1 2 13 15
+
+framework_failure=0
+mkdir -p $tmp || framework_failure=1
+cd $tmp || framework_failure=1
+
+if test $framework_failure = 1; then
+  echo "$0: failure in testing framework" 1>&2
+  (exit 1); exit 1
+fi
+
+PROG=`echo $0|sed 's,.*/,,'`; export PROG
+
+$PERL -e 1 > /dev/null 2>&1 || {
+  echo 1>&2 "$0: configure didn't find a usable version of Perl," \
+    "so can't run this test"
+  exit 77
+}
+
+exec $PERL -w -I$pwd/.. -MCoreutils -- - <<\EOF
+require 5.003;
+use strict;
+
+(my $program_name = $0) =~ s|.*/||;
+
+# Turn off localisation of executable's ouput.
address@hidden(LANGUAGE LANG LC_ALL)} = ('C') x 3;
+
+my @Tests =
+    (
+     # Skip the exact length of the input file.
+     # Before coreutils-6.10, this would ignore the "-j 1".
+     ['j-bug1', '-c -j 1 -An', {IN=>{g=>'a'}}, {OUT=>''}],
+     ['j-bug2', '-c -j 2 -An', {IN=>{g=>'a'}}, {IN=>{h=>'b'}}, {OUT=>''}],
+     # Skip the sum of the lengths of the first three inputs.
+     ['j-bug3', '-c -j 3 -An', {IN=>{g=>'a'}}, {IN=>{h=>'b'}},
+                                               {IN=>{i=>'c'}}, {OUT=>''}],
+     # Skip the sum of the lengths of the first three inputs, printing the 4th.
+     ['j-bug4', '-c -j 3 -An', {IN=>{g=>'a'}}, {IN=>{h=>'b'}},
+                              {IN=>{i=>'c'}}, {IN=>{j=>'d'}}, {OUT=>"   d\n"}],
+    );
+
+my $save_temps = $ENV{DEBUG};
+my $verbose = $ENV{VERBOSE};
+
+my $prog = $ENV{PROG} || die "$0: \$PROG not specified in environment\n";
+my $fail = run_tests ($program_name, $prog, address@hidden, $save_temps, 
$verbose);
+exit $fail;
+EOF
--
1.5.3.rc4.67.gf9286