So today I'm a little bit lazy and I've got the day off work. As my previous plan suggested I wanted to spend at least some of the day tackling semi-random bugs. Earlier I picked a victim: less.
less rocks, and I use it daily. I even wrote an introduction to less once upon a time.
So lets take a look at two bugs from the long-neglected pile. These two issues are basically the same:
They seem like simple ones to fix, with the same root cause. Here's an example if you want to play along at home:
cp /dev/null testing gzip testing zless testing.gz
What do you see? I see this:
"testing.gz" may be a binary file. See it anyway?
When I select "y" I see the raw binary of the compressed file.
So, we can reproduce it. Now to see why it happens. /bin/zless comes from the gzip package and is a simple shell script:
#!/bin/sh # snipped a lot of text LESSOPEN="|gzip -cdfq -- %s"; export LESSOPEN exec less "$@"
So what happens if we run that?
$ LESSOPEN="|gzip -cdfq -- ~/testing.gz" /usr/bin/less ~/testing.gz "/home/skx/testing.gz" may be a binary file. See it anyway?
i.e. it fails in the same way. Interestingly this works just fine:
gzip -cdfq -- ~/testing.gz | less
So we've learnt something interesting and useful. We've learnt that when LESSOPEN is involved we get the bug. Which suggests we should "apt-get source less" and then "rgrep LESSOPEN ~/less-*/".
Doing so reveals the following function in filename.c:
public char *
open_altfile(filename, pf, pfd)
char *filename;
int *pf;
void **pfd;
{
/* code to test whether $LESSOPEN is set, and attempt to run the
command if it is */
/*
* Read one char to see if the pipe will produce any data.
* If it does, push the char back on the pipe.
*/
f = fileno(fd);
SET_BINARY(f);
if (read(f, &c, 1) != 1)
{
/*
* Pipe is empty. This means there is no alt file.
*/
pclose(fd);
return (NULL);
}
ch_ungetchar(c);
*pfd = (void *) fd;
*pf = f;
return (save("-"));
That might not be digestible, but basically less runs the command specified in $LESSOPEN. If it may read a single character of output from that command it replaces the file it was going to read with the output of the command instead!
(i.e. Here less failed to read a single character, because our gzipped file was zero-bytes long! So instead it reverted to showing the binary gzipped file.)
So we have a solution: If we want this to work we merely remove the "read a single character test". I can't think of circumstance in which that would do the wrong thing, so I've submitted a patch to do that.
Bug(s) fixed.
Incidentally if you like these kind of "debuggin by example" posts, or hate them, do let me know. So I'll know whether to take notes next time or not..
And thanks for the fix(shs) !
The little bit of programming that I have done has had to be small enough that I can picture the whole thing in my head. Looking at other peoples code I find really hard.
One day, I would like to squash a bug :-)
Thanks for the (positive) feedback!
As for why the "cp /dev/null" command, rather than touch there was a similar line in the bug report. So I just adapted that.
Generally I'd use touch - actually I'm suprised I didn't change it, I guess I was too busy thinking about how to reproduce the problem with the least effort!
I didn't test the documentation to see if this was expected behaviour - but I did test that otherwise things work as expected.
Certainly "eval $(lessfile)" and "eval $(lesspipe)" both continue to allow normal and .gz files to be read.
Thanks, Josh
An idea I have been pondering for a while is how to make life easier for people who want to debug a more complex program. The standard response is "build a debug version" but not many end users know how to do that in a sane environment (that would allow the package maintainer to see if the problem is in the package or the environment it's being run in).
One way to address this is for debian to make -dbg packages available on request; if they're built on the buildds, they should be an exact match for the problematic package.
For extra points, one could build such packages so they provide reports automagically, along the lines of this project - http://www.cs.wisc.edu/cbi/. I have no idea how feasible this is. For one thing it has potential to vastly inflate the debian archive if not done selectively.