Bug 805735 - Foption patch depends on uninitialised variable when reading from stdin and also implies -X option
Foption patch depends on uninitialised variable when reading from stdin and a...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: less (Show other bugs)
17
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Vojtech Vitek
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-21 19:21 EDT by Colin Guthrie
Modified: 2015-03-04 18:57 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-05-14 12:42:59 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Colin Guthrie 2012-03-21 19:21:26 EDT
The patch in the F17 and rawhide less packages to fix up Foption is broken in a couple different ways.

I don't personally use Fedora, but I noticed this while reviewing your patches for inclusion in Mageia.

Here is a description I previously sent to Vojtech Vitek. He requested I open a bug to track this.



I was just debugging some annoying behaviour in "less" last night in
Mageia. Years ago (c. 2005) we, ahem, borrowed the Foption patch from
RedHat.

As I periodically look for improvements etc, I checked the current
Fedora version of this patch.

http://pkgs.fedoraproject.org/gitweb/?p=less.git;a=blob;f=less-443-Foption.patch;h=d657d4455a0e92495c5ff5c6347a17a03a90f204;hb=HEAD

In my use case however, I could not make it work.

I wanted to use -FR as my default options, but no matter what I tried,
the -F option always seemed to also imply the -X option too.

The -X option sadly has the side effect of breaking mouse scroll wheel
integration while using less under e.g. gnome-terminal.


So I dug into the code a little bit and found two bugs with the above patch.

The first is that the line count function always returned 0. It seems
that position(TOP) call always returned the NULL_POSITION thus never
counted any lines.

Secondly the value for line_count was only initialised when working with
files, not when working with stdin, so the behaviour in that case would
have been non-deterministic based on whatever random memory value was in
the variable.

The patch should fix it up and seems to work for me:

http://svnweb.mageia.org/packages/cauldron/less/current/SOURCES/less-445-Foption-fix.patch?revision=224291&view=markup


Any further review would be appreciated.
Comment 1 Vojtech Vitek 2012-05-14 12:42:59 EDT
Hi Colin,

thank you for your patch. I've reviewed it briefly and tested as well.

I've merged the changes into the original Foption patch and created the Foption.v2.patch, see http://pkgs.fedoraproject.org/gitweb/?p=less.git;a=blob;f=less-444-Foption.v2.patch;h=eb9140da8e4714e2dc7e423f56416992fd315c5f;hb=47da4db53ff5f00cfa893e05c9995959b8491242

Let's see how the Fedora community accepts the -F behavior now while testing Rawhide.
Comment 2 Colin Guthrie 2012-05-15 05:51:23 EDT
Annoyingly my colleague pointed out a small problem with my patch...

It relates to low-flow data...

(i=1; while [ $i -lt 50 ]; do echo "This is line $i"; i=$((i + 1)); sleep 0.5; done) | less

vs.

(i=1; while [ $i -lt 50 ]; do echo "This is line $i"; i=$((i + 1)); sleep 0.5; done) | less -F


In the latter case it waits for 0.5*21 = 11s before displaying any of the data.

Due to the kind of data I work with and the places where -F is used, I personally don't have a problem with this, but there should really be more code added to cope with this kind of situation :s
Comment 3 Colin Guthrie 2012-05-15 05:52:09 EDT
(PS I was rounding up before you point out my lack of math skills :p)
Comment 4 Vojtech Vitek 2012-05-15 14:25:12 EDT
(In reply to comment #2)
> (i=1; while [ $i -lt 50 ]; do echo "This is line $i"; i=$((i + 1)); sleep 0.5;
> done) | less -F
> 
> In the latter case it waits for 0.5*21 = 11s before displaying any of the data.

I can't reproduce this. Could you please test the update packages, eg. less-444-7.fc18? Thanks!
Comment 5 Colin Guthrie 2012-05-17 04:30:28 EDT
Hmm, I don't have an handy Fedora install sadly, but I tested the patch itself and I can still reproduce it with that applied rather than the previous two patches I had. We do however have a newer version of less than you guys (445 vs 444) so that may explain the difference in behaviour.

That said, I can quite easily see in the patched code how this happens: The get_line_count() function will always count up to as many lines as the screen is high. If the input is coming from stdin, and there has not yet been an EOF, it will wait until that many lines exist, thus delaying display of ANY output until the screen is full.

In an ideal world it would start displaying (almost) immediately and only switch to the scrolly output when the screen is full.


Anyway, I'm using this as a "less" bug tracker rather than a fedora one so that's likely not overly helpful to you. I can bow out of the discussion if you like (especially if you cannot reproduce)?

Note You need to log in before you can comment on or make changes to this bug.