[Home]

Summary:ASTERISK-15652: [patch] asterisk command history loads as unusable garbage
Reporter:jw-asterisk (jw-asterisk)Labels:
Date Opened:2010-02-17 21:19:19.000-0600Date Closed:2011-06-07 14:08:24
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 0005-Build-using-external-libedit.patch
( 1) 0006-Fix-history-loading-when-using-external-libedit.patch
Description:When using asterisk interactively with command interface a history file is created (~/.asterisk_history). Lines written to the history file are white-space encoded (in ast_el_write_history:history:case H_SAVE:history_save:strvis).

But when the history file is loaded it reverse (strunvis) is not performed.  Therefore the history becomes unusable garbage.

****** STEPS TO REPRODUCE ******

asterisk -r
<type a few commands containg spaces>
quit

asterisk -r
<use up-arrow key to view some earlier history>


****** ADDITIONAL INFORMATION ******

Instead of using history(el_hist, &ev, H_LOAD, filename) for some strange reason the function ast_el_read_history()has been written with a very broken partial parsing of the history file.

Shouldn't it look much like ast_el_write_history() and invoke history(...H_LOAD...)?

Obviously whoever wrote that history load function has never tested it or even used it.
Comments:By: John Todd (jtodd) 2010-03-03 08:53:35.000-0600

History works fine on MacOS with 1.6.2.  Can you give a bit more information on what version and OS you're on? I don't doubt it's broken, but knowing what the details are would help a bit.
By: Leif Madsen (lmadsen) 2010-03-03 08:55:25.000-0600

I've also verified this is working for Asterisk trunk on my Ubuntu 9.10 OS.
By: jw-asterisk (jw-asterisk) 2010-03-03 16:19:35.000-0600

Confirmed that this is broken with 1.6.2.5

Imadsen ... you don't even say what version installs on Ubuntu 9.10 OS.  That isn't very helpful.

jotdd - you doubt it's broken?  Why do you doubt it? Look, asterisk is full of bugs and always has been.  So why can there be some doubt purely on the basis that asterisk never has bugs?

Here is the offending code out of 1.6.2.5 - see it doesn't reverse the encoded characters:

> static int ast_el_read_history(char *filename)
> {
>        char buf[MAX_HISTORY_COMMAND_LENGTH];
>        FILE *f;
>         int ret = -1;
>
>        if (el_hist == NULL || el == NULL)
>                ast_el_initialize();
>
>        if ((f = fopen(filename, "r")) == NULL)
>                return ret;
>
>        while (!feof(f)) {
>                if (!fgets(buf, sizeof(buf), f))
>                        break;
>                if (!strcmp(buf, "_HiStOrY_V2_\n"))
>                        continue;
>                if (ast_all_zeros(buf))
>                        continue;
>                if ((ret = ast_el_add_history(buf)) == -1)
>                        break;
>        }


By: jw-asterisk (jw-asterisk) 2010-03-03 17:06:42.000-0600

jtodd - it isn't the history itself that is broken.  It is the loading of history from the saved ~/.asterisk_history file.  You need to exit asterisk then start afresh with "asterisk pr" and immediately try going back in history.
By: Jason Parker (jparker) 2010-03-03 17:28:54.000-0600

Rather than misread what you're being told and harassing the bug marshals, how about we work to move this forward?  You've not answered the questions you've been asked yet.  This is absolutely the wrong way to try to get help.

Please describe the behavior you are seeing, and give us some information about your platform.

I would also highly recommend going back and re-reading what jtodd and lmadsen said.
By: jw-asterisk (jw-asterisk) 2010-03-03 17:38:52.000-0600

jtodd said: "History works fine on MacOS 1.6.2".  He did not mention the minor revision.  He did not mention whether retrieval of saved history works ... just that history works.  He didn't mention what localized patches might have been applied.

imadsen didn't indicate what version was working for him.

The answer to the questions have already been supplied.  The bug report states what version it relates to.  The platform is Linux Fedora Core 12 but it has been compiled from new source (not the fc12 supplied version), so the platform is largely irrelevant.

For goodness sake ... if this is the attitude taken when there is a blatant bug (I suppose nobody has bothered to even read the source - it is so obvious) then no wonder asterisk is so bug-ridden.

FYI I have already patched my asterisk to fix the bug - basically by replacing the body of funtion ast_el_read_history() with:

<pre>
> static int ast_el_read_history(char *filename)
> {
>       HistEvent ev = {0};
>
>        if (el_hist == NULL || el == NULL)
>                ast_el_initialize();
>       return (history(el_hist, &ev, H_LOAD, filename))
>}
</pre>

And, of course, it fixed the problem!

Is that any help?
By: Jason Parker (jparker) 2010-03-03 17:53:01.000-0600

>> jtodd said: "History works fine on MacOS 1.6.2". He did not mention the minor
>> revision. He did not mention whether retrieval of saved history works ... just
>> that history works. He didn't mention what localized patches might have been
>> applied.
>>
"History works" means that all of it works for him.  When somebody gives the name of a branch they've tested, it means they were testing on the latest revision of that branch.

>> imadsen didn't indicate what version was working for him.
>>
He absolutely did.  He said "Asterisk trunk".

>> The answer to the questions have already been supplied. The bug report states
>> what version it relates to. The platform is Linux Fedora Core 12 but it has
>> been compiled from new source (not the fc12 supplied version), so the platform
>> is largely irrelevant.
>>
You did not supply the platform until just now.  Yes, it is very relevant.  You still have yet to actually describe the issue you're seeing.

>> For goodness sake ... if this is the attitude taken when there is a blatant
>> bug (I suppose nobody has bothered to even read the source - it is so obvious)
>> then no wonder asterisk is so bug-ridden.
>>
Neither jtodd nor lmadsen gave you any attitude.  However, if you're going to flame our bug marshals and developers while not giving us any useful information - yes, I am going to call you on it.

>> FYI I have already patched my asterisk to fix the bug - basically by replacing
>> the body of funtion ast_el_read_history() with:
>>
SNIP

>> And, of course, it fixed the problem!
>>
>> Is that any help?
>>
Not in the least.  Once again - you have not even described the issue you are seeing.
By: jw-asterisk (jw-asterisk) 2010-03-03 17:57:39.000-0600

> Once again - you have not even described the issue you are seeing.

What a load of crap!!  The issue was fully described in the initial bug report.  Precisely and completely.

How can you espouse such utter nonsense?  Didn't you even read the bug report?  Obviously not!
By: Jason Parker (jparker) 2010-03-03 18:01:44.000-0600

"the history becomes unusable garbage" is not a useful description.

What *SPECIFICALLY* are you seeing?
By: jw-asterisk (jw-asterisk) 2010-03-03 18:05:42.000-0600

Given that I have already patched asterisk I will have so go from memory:

Something like:

> core\040\040set\040debug

Instead of

> core set debug

But after exiting and starting again it becomes:

> core\040\040\040set\040\040\040debug

or worse.

But you have to go back in history immediately after entering asterisk command-line.  Current history is always ok.  Only history retrieved from ~/.asterisk_history.

Please ... this is really simple.  Just inspecting the code will lead to to the problem!
By: Jason Parker (jparker) 2010-03-03 18:12:56.000-0600

That description is much more useful.

What terminal application are you using?  The output of `echo $TERM` would be good too.

It seems there may be something about your system that is causing you to see this and not others.  I have no doubt that your comment about not using strunvis is correct, but we need to be able to reproduce this.

Is there anything you're aware of that might be considered "unusual" about your system that could lead to reproducing this?
By: jw-asterisk (jw-asterisk) 2010-03-03 18:17:16.000-0600

xterm "terminal".

Nothing unusual.

Perhaps there is something "unusual" about your system which somehow automatically translates incoming "\040" into spaces?  That is, assuming that you have tested against this bug report.
By: jw-asterisk (jw-asterisk) 2010-03-03 18:18:34.000-0600

Why don't you type, from bash, 'echo "\040"'.

What do you get?  I get '\040' except for the quotes.
By: Sean Bright (seanbright) 2010-03-03 19:24:58.000-0600

Why don't you type, from bash, 'printf "\040"'.

What do you get? I get ' ' except for the quotes.
By: Bradley Watkins (marquis) 2010-03-03 19:50:50.000-0600

I am unable to reproduce this behavior on Fedora 12 (there is no such thing as Fedora Core 12, they dropped the "Core" nomenclature several versions ago) using xterm, lxterminal, xfterm4, or Konsole.

I've tried all combinations of the above terminals using TERM=xterm, TERM=vt100, and TERM=vt220 in both the latest 1.6.2 branch revision and trunk.
By: jw-asterisk (jw-asterisk) 2010-03-03 20:13:15.000-0600

Your inability to reproduce the problem doesn't detract from the fact that there is defective code in asterisk.

The fact that you get '&nbsp;' from the echo test indicates that something (your system's tty driver perhaps) is masking the bug.

You already have the final answer (the bug fix), you already know your own different echo test response, so why can't you draw the only logical conclusion?


By: jw-asterisk (jw-asterisk) 2010-03-03 20:21:14.000-0600

I repeat: for the echo test I get "\040" from a recent install of "fedora 12" on the console screen (tty1) with TERM set to "linux".

In any case, asterisk has a bug whereby it does not reverse the encoding effect that it performs when saving the history file.  You already know this.

It really doesn't matter what exactly is masking the bug from you ... you have indirect evidence that under certain conditions the bug is apparent.

Why waste everyone's time ... just patch the fix!
By: Leif Madsen (lmadsen) 2010-03-03 20:24:15.000-0600

Wow, I'm not entirely sure what set you off, but the intention was certainly not to do that. I quote jtodd:

"History works fine on MacOS with 1.6.2. Can you give a bit more information on what version and OS you're on? I don't doubt it's broken, but knowing what the details are would help a bit."

He states, "I don't doubt it's broken, but knowing what the details are would help a bit", this seems to be a statement saying he wasn't able to reproduce the issue immediately, but that some additional information might be useful in trying to determine why you're experiencing the issue and he is not.

I stated I was not able to reproduce this on Ubuntu 9.10 with Asterisk trunk, which is certainly a version of Asterisk -- it's the latest non-branched code.

If you were able to so easily reproduce the issue, and patch it yourself, I'm not entirely sure why you wouldn't just provide the patch in the first place, in which case it would likely be more obvious as a developer would be able to see what part of the code needed to be modified, which likely would have made the issue as obvious as you expected it to be.

Hopefully in the future you'll be more willing to work with the bug marshals and developers, as the intention was never to belittle or antagonize you, but rather to just get some additional information about how to reproduce an issue you were experiencing so we could get it resolved. Neither jtodd or myself were able to reproduce the issue immediately in the few moments we had to triage your issue, which is why we were looking for additional information to move this forward.

You'll catch more flies with honey than with vinegar.
By: jw-asterisk (jw-asterisk) 2010-03-03 20:40:22.000-0600

You should only ever get a single space output when you do "echo -e '\040'".  That is why echo has a "-e" option.  If you are getting the escape interpreted all of the time then you have another serious bug elsewhere in your system (shell, glibc, tty driver, or whatever).

Hopefully in the future you will be more willing to look at the portion of code that I clearly identified in my initial bug report, and properly digest the supplied information, before leaping into denial mode.

Let he who is without sin cast the first stone.
By: jw-asterisk (jw-asterisk) 2010-03-03 20:45:08.000-0600

What do you get from "shopt xpg_echo"?

Is it on or off?


By: Bradley Watkins (marquis) 2010-03-04 07:42:12.000-0600

"I repeat: for the echo test I get "\040" from a recent install of "fedora 12" on the console screen (tty1) with TERM set to "linux"."

Actually, this is NOT a repeat of any kind.  You had not previously stated the value of your TERM variable, nor that you were using tty1.  In fact, by stating you were using xterm previously you implied that you were not using a tty at all.

FWIW, I do get the literal '\040' from both my desktop system (F12, recently updated) and a freshly-installed F12 system when I perform your echo test as stated.

I do not, however, see the effect of the issue you have reported in the Asterisk console from *any* combination of these two systems, TERM settings(linux, xterm, vt100, vt220 were tried), actual terminal apps/terminals (tty1, lxterminal, Konsole, xfterm4, xterm), or Asterisk version (1.6.2 branch, trunk).

A default Fedora system has xpg_echo off, as do both of my systems.  I have tried with it on, however, and for any of the permutations I tried it made no difference.

There is obviously something different with your system (I'm not saying wrong, just different) from a base F12 install, and I would be interested in knowing what that is both so that I can reproduce the issue you are seeing as well as run other tests with the Asterisk console to know if there are other issues.

All of that said, I have tested with the provided new version of the ast_el_read_history function and on both of my systems it seems to work just fine.
By: jw-asterisk (jw-asterisk) 2010-03-04 07:49:39.000-0600

But wait ... I thought it was reported that the echo test always resulted in a space on your system?  Or was that just everybody else's systems.

My system is probably correctly configured and the others are not.

That is because the xpg_echo setting performs correctly. With it off I get the correct result - it echoes as \040. It would appear that the other systems are misconfigured because they always incorrectly translates the \040 characters as a space.

Also, on my system there is a difference between the action of "echo" and "echo -e".

It really is up to others to find out what is wrong with their systems.  It isn't up to me.  My system is correct.  Others are misconfigured.  The evidence is in, and has demonstrated clearly that those other systems are flawed.


By: jw-asterisk (jw-asterisk) 2010-03-04 07:54:34.000-0600

BTW on my system turning xpg_echo on certainly creates a different behavior ... with the echo test produces a single space.

If on your system it doesn't make any difference then clearly you have problems somewhere.
By: jw-asterisk (jw-asterisk) 2010-03-04 08:00:01.000-0600

I will also clarify something that might be of further interest to you.

My initial echo test was done on an xterm.  It goes without saying that the TERM setting for an xterm is generally xterm.

See here: <https://issues.asterisk.org/view.php?id=16858#118926>

I made a subsequent test on the console for an additional check. So I was repeating the result, albeit under slightly different conditions.

But it was good doing so because it provided you with another prefect opportunity to attack me.
By: Leif Madsen (lmadsen) 2010-03-04 08:04:45.000-0600

OK, I think this has been explained to death. A developer will get to this as soon as time, resources, and priority allow.
By: Bradley Watkins (marquis) 2010-03-04 08:11:36.000-0600

No, I believe that might have been someone else.  I never said that.

As for "correctly" configured, then your assertion is that the base Fedora install from DVD media is flawed?  Then we should report a bug to them so that it may be corrected.

On my systems there is indeed a difference between "echo" and "echo -e" as is the case for you.  So they appear to be operating correctly in that regard at least.

Anyway, it's not really up to me to find anything out about any systems because mine all work fine with and without your version of the function.  I was just hoping to determine the differences to try and help out the Asterisk project.
By: Sean Bright (seanbright) 2010-03-04 09:30:16.000-0600

What is the output of the following:
<pre>$ cd /path/to/asterisk/source
$ egrep -C10 strvis main/editline/history.c<pre>


By: jw-asterisk (jw-asterisk) 2010-03-04 18:01:03.000-0600

A question - why is your name, Marguis, annotated as being the "reporter"?  My name also bears that label and both cannot be "reporter" for this bug surely?

And to seanbright:
  - why use egrep when grep would do?
  - and why use the -r flag on a single file? You cannot recurse a file.
  - and why use the -i flag when you wish to locate the exact string 'strvis'?
I am ignoring your request, seanbright (manager?!), because it seems so badly constructed, displays a poor understanding of grep/egrep, and really serves absolutely no purpose what-so-ever because I have already indicated which version of asterisk is involved.  Do you seriously expect to discover something different to what shows in svn or the current source.

BTW I would venture to suggest that asterisk's instability derives from "too many hands" as evidenced in this thread, and from the lack of a single strong developer or two who take effective/dominant ownership of the source.


By: Russell Bryant (russell) 2010-03-04 21:05:50.000-0600

reporter/manager/administrator represents an access level for the issue tracker.  It does not represent a role within a given issue.

Regarding Sean's request, it was simply to find out if you had made any modifications to that part of the code (specifically, the code that writes out the history).
By: Sean Bright (seanbright) 2010-03-04 21:14:29.000-0600

Fair enough, thank you for your report.

For record keeping purposes:
The reporter indicates in his report that strvis() is being called on entries that are being written to the history file.  While this is true, the result of strvis() is being completely ignored - looking at the source of main/editline/history.c in asterisk 1.6.2.5 (one of the versions of asterisk the reporter mentions as being affected) reveals the following code on lines 666 and 667 in history_save (the location mentioned by the reporter):

666    (void) strvis(ptr, ev.str, VIS_WHITE);
667    (void) fprintf(fp, "%s\n", ev.str);

(You can see this here: http://svnview.digium.com/svn/asterisk/tags/1.6.2.5/main/editline/history.c?view=markup )

You'll notice that the first argument to strvis - ptr - is where the whitespace encoded string will be written.  ev.str is the source string.  You'll also notice, unfortunately, that the fprintf on the next line uses ev.str as well instead of the whitespace encoded string in ptr.  This means that the string being written to the history file is not whitespace encoded and will be the raw history command that we expect when reading the history entries back in.  So while there may be an underlying bug in editline, we are not affected by it because of the way we read the history entries back out of the file.  Of note is that this has been resolved upstream for quite some time:

http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libedit/history.c#rev1.20

We may want to upgrade at some point, but this is not critical since we have no open or valid reports relating to it.

All of this, coupled with the fact that no one else is able to reproduce this leads me to believe it is a problem specific to the reporter.

Closing.


By: jw-asterisk (jw-asterisk) 2010-03-04 21:41:24.000-0600

But, despite what you say about strvis and the encoded string not being used, this is what is in my asterisk history file:

<pre>
_HiStOrY_V2_
dadhi\040show\040status
dahdi\040show\040status
quit
dahdi\040show\040status
quit
core\134040set
core\134\134040set
quit
<pre>

So how has that encoding happened?
By: jw-asterisk (jw-asterisk) 2010-03-04 22:08:45.000-0600

Ok, so it would appear that my version is picking up history() from elsewhere.  The main/editline/libedit.a is not built.

So, since configure might somehow decide to use existing glibc readline's history() (or whatever happens) then it is important for the optional inbuilt editline's history() to functionally match exactly.

Checked with gdb and definitely there is no strvis() in the binary.

So it is important that saving and loading history both invoke history() otherwise symmetric processing of the history file wont happen.
By: jw-asterisk (jw-asterisk) 2010-03-04 23:13:55.000-0600

Can definitely confirm than am linking against external version of libedit.

This means that this bugs exists only when linking against external libedit.  When using the internal libedit there is an additional bug (the way strvis is used) which coindicentally negates the other bug.

Bug with external libedit then the bug is exposed.

Therefore best to use my supplied version of ast_el_read_history() and then it will work properly with internal and with external libedit.
By: Russell Bryant (russell) 2010-03-04 23:28:59.000-0600

Have you made some modifications to make it not build the internal libedit?  The Makefile explicitly builds and links in the internal one.
By: jw-asterisk (jw-asterisk) 2010-03-05 00:19:19.000-0600

No, just reused a Fedora 12 rpm script.

In actual fact Fedora applies a patch (0005-Build-using-external-libedit.patch) which does what says.

And I have to say that the Fedora scheme for naming that patch - without any mention of asterisk - is really quite clueless.  They name all of the patches 0000-xxxx etc which clearly doesn't form any association with asterisk. And they clearly expose the patch to name clash with any of the tens of thousands of other applications and their patches.

In any case it would still be best practice to ensure that any copy of a potentially external library should only be an failsafe option and that it should functionally match that external version.

The external libedit does encode whitespace, and there is a bug in the asterisk version which has prevented whitespace from being encoded.  So for compatability whitespace should be encoded, and the instance of strvis() should be repaired, as should ast_el_read_history().
By: Jeffrey C. Ollie (jcollie) 2010-03-05 10:01:52.000-0600

I've just uploaded the patch in question that builds asterisk against an external copy of libedit.
By: Sean Bright (seanbright) 2010-03-05 10:15:13.000-0600

Thank you for your report.

After some discussion we have determined that this is a bug in the patches applied by the Fedora project.  This issue has been submitted independently to the Fedora project:

https://bugzilla.redhat.com/show_bug.cgi?id=560333

As for the Asterisk project, we believe we have identified the correct approach to move this forward.

1) The internal version of editline used by Asterisk should be synced with the latest upstream.
2) Asterisk should be modified to be able to link against an external editline library.
3) The build system should fall back on (1) if (2) is not found.

The Fedora packager has graciously agreed to submit a patch for #2.  1 and 3 are feature requests and will be handled when developer resources are willing and able.  If you would like to contribute, please create a new issue and attach the necessary patches.
By: Jared Smith (jsmith) 2010-03-05 10:19:22.000-0600

By way of clarification, let me add a couple of points:

In the short term, the easiest way to solve the reported problem is to have Fedora include a patch (similar to the one you suggested) along with their patch set, as it's their patch that's triggering the behavior.

In the long term, steps 1 and 2 and 3 in the note above by seanbright will be done, but because they're larger in scope, they're being considered as "feature requests" and not as bugs.  That means they won't be applied to release branches (such as 1.6.2), but will make it into the next release branch after the work has been completed.

Make sense?
By: Jeffrey C. Ollie (jcollie) 2010-03-05 11:26:11.000-0600

Aaaand, I've uploaded a patch that will fix the history problem in Fedora.  I'm not going to do a special build just for this patch but it'll start showing up in packages as new versions of Asterisk are released.

I'm going to test this out with the internal editline library and if it works I really think that it should be included with the released branches as it actually reduces the amount of code.
By: Jeffrey C. Ollie (jcollie) 2010-03-05 13:43:53.000-0600

My testing shows that 0006-Fix-history-loading-when-using-external-libedit.patch works both when using the internal editline library and when linking an external libedit.
By: Digium Subversion (svnbot) 2010-07-27 16:57:06

Repository: asterisk
Revision: 280019

U   branches/1.8/build_tools/menuselect-deps.in
U   branches/1.8/configure
U   branches/1.8/configure.ac
U   branches/1.8/include/asterisk/autoconfig.h.in
U   branches/1.8/include/asterisk/term.h
_U  branches/1.8/main/
U   branches/1.8/main/Makefile
U   branches/1.8/main/asterisk.c
U   branches/1.8/main/cli.c
D   branches/1.8/main/editline/
U   branches/1.8/main/term.c
U   branches/1.8/main/xmldoc.c
U   branches/1.8/makeopts.in

------------------------------------------------------------------------
r280019 | seanbright | 2010-07-27 16:57:05 -0500 (Tue, 27 Jul 2010) | 23 lines

Add ability to use system libedit and update bundled libedit.

The version of libedit that is bundled with asterisk is old and has some bugs.
This patch updates the bundled version of libedit within asterisk, and also
updates asterisk to use the system libedit instead if one is available (and
pkg-config is available).  This review integrates several patches from other
users specifically kkm and tzafrir.

(closes issue ASTERISK-14859)
Reported by: kkm
Patches:
     015929-astcli-editrc-trunk.240324.diff uploaded by kkm (license 888)

(issue ASTERISK-15652)
Reported by: jw-asterisk

(closes issue ASTERISK-15821)
Reported by: tzafrir
Patches:
     0001-allow-using-system-copy-of-libedit.patch uploaded by tzafrir (license 46)

Review: https://reviewboard.asterisk.org/r/807/

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=280019