About Archive Tags RSS Feed

 

Listen to me when I'm telling you

14 February 2008 21:50

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..

| 22 comments

 

Comments on this entry

icon Ondrej at 13:40 on 14 February 2008
Yes, please continue posting debugging posts, it's very useful.
icon plagal at 13:50 on 14 February 2008
Well, I like them. :-)
icon Michael at 14:18 on 14 February 2008
Very intresting, thanks :)
icon Jonathan Ballet at 14:23 on 14 February 2008
I Just wanted to say that I like these kind of posts :)
And thanks for the fix(shs) !
icon Adam at 14:32 on 14 February 2008
I like these types of posts. They're very informative. Thanks for walking us through the way you diagnose, debug and fix a problem. Cheers from the U.S.!
icon Philip Stubbs at 14:51 on 14 February 2008
I like these posts. I am not a coder, and it is very interesting to see how you are able to locate the exact location in the source code, and identify what it does.
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 :-)
icon Amaya at 14:58 on 14 February 2008
I loved this "debuggin by example" post, keep them coming!
icon Jaime at 16:09 on 14 February 2008
Yup, I agree too: these debugging posts are excellent. PS why "cp /dev/null testing" rather than "touch testing"? Is it just personal preference? :-)
icon Steve at 16:19 on 14 February 2008

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!

icon Marius Gedminas at 16:40 on 14 February 2008
Removing that check breaks a documented feature of less. Have you looked at the "INPUT PREPROCESSOR" section of the less(1) manual page? Have you tested that less regularfile.txt still works when you do eval `lesspipe`?
icon Steve at 16:50 on 14 February 2008

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.

icon Jeff at 18:55 on 14 February 2008
I like the debugging by example posts as well. For someone like me who would like to be involved, but isn't. They give a great sense of what being involved looks like.
icon Josh at 19:02 on 14 February 2008
Great post. Please keep up the "debugging by example posts". I really want to become more involved in the community and posts like this make it evident how to get started.
Thanks, Josh
icon Saint Aardvark at 19:27 on 14 February 2008
I like this post a lot. More Debian debugging, please!
icon Alex at 23:26 on 14 February 2008
Call me a sadist, but I just love watching you crush those little critters. :)
icon xipmix at 00:54 on 15 February 2008
This was indeed useful.
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.
icon eric at 08:34 on 15 February 2008
Yes, interesting post. I'm not a fan of C code but the example you provide is clear.
icon Kai at 16:50 on 15 February 2008
As everyone can learn from others I'd appreciate if you would continue this sort of posts. Thanks in advance. Kind regards, Kai
icon George at 11:09 on 16 February 2008
Just wanted to say that I find this interesting, so please do keep posting them. Thanks.
icon lters at 16:33 on 18 February 2008
Please keep the debug posts coming!
icon Daf at 16:39 on 19 February 2008
More of this sort of thing.
icon Torbjorn at 13:23 on 18 February 2008
I find your debugging by example posts very interesting, and very easy to learn from .. please keep them coming.