Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#12282 closed defect (fixed)

Fix strcmp() with NULL argument in termcap library

Reported by: jdemeyer Owned by: tbd
Priority: blocker Milestone: sage-4.8
Component: packages: standard Keywords: sd35.5
Cc: Merged in: sage-4.8.rc0
Authors: Jeroen Demeyer Reviewers: Georg S. Weber
Report Upstream: None of the above - read trac for reasoning. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

If the environment variable TERMCAP is set and TERM is not set, then calling tgetent() from the termcap library produces a Segmentation Fault because it calls strcmp() with a NULL argument. Sage hits this problem because of readline, see #11970.

To reproduce this issue: let tgetent_test.c be the file

#include <termcap.h>
#include <stdlib.h>
#include <stdio.h>

int main(int argc, char** argv)
{
    char* buf = malloc(100000);
    tgetent(buf, "dumb");
    printf("Successful!\n");
}

Now compile and link this with Sage's libtermcap.a (don't use -ltermcap, instead explicitly specify the path to the libtermcap.a file):

$ gcc tgetent_test.c local/lib/libtermcap.a -o tgetent_test

Run this with TERMCAP set but TERM unset:

$ unset TERM
$ export TERMCAP="x"
$ ./tgetent_test
Segmentation fault

Not reported upstream because there is no known contact person nor bug report page.

Apply: http://boxen.math.washington.edu/home/jdemeyer/spkg/termcap-1.3.1.p2.spkg

Attachments (1)

termcap-1.3.1.p1-p2.diff (4.8 KB) - added by jdemeyer 8 years ago.

Download all attachments as: .zip

Change History (35)

comment:1 follow-up: Changed 8 years ago by zimmerma

  • Status changed from new to needs_info

Jeroen, I cannot reproduce this issue:

tiramisu% cat tgetent_test.c
#include <termcap.h>
#include <stdlib.h>

int main(int argc, char** argv)
{
  char* buf = malloc(100000);
  tgetent(buf, "dumb");
}
tiramisu% gcc tgetent_test.c -o tgetent_test -ltermcap
tiramisu% printenv | grep TERM
TERMCAP=1
tiramisu% ./tgetent_test
tiramisu%

Paul

comment:2 Changed 8 years ago by zimmerma

  • Keywords sd35.5 added

comment:3 in reply to: ↑ 1 Changed 8 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Status changed from needs_info to needs_review

comment:4 Changed 8 years ago by jdemeyer

The fix in the spkg is

  • termcap.c

    diff -ru src/termcap.c src.fixed/termcap.c
    old new  
    460460  char *tcenv = NULL;          /* TERMCAP value, if it contains :tc=.  */
    461461  char *indirect = NULL;       /* Terminal type in :tc= in TERMCAP value.  */
    462462  int filep;
     463  char *term_name;
    463464
    464465#ifdef INTERNAL_TERMINAL
    465466  /* For the internal terminal we don't want to read any termcap file,
     
    500501     it is the entry itself, but only if
    501502     the name the caller requested matches the TERM variable.  */
    502503
    503   if (termcap_name && !filep && !strcmp (name, getenv ("TERM")))
     504  term_name = getenv("TERM");
     505  if (termcap_name && !filep && term_name && !strcmp (name, term_name))
    504506    {
    505507      indirect = tgetst1 (find_capability (termcap_name, "tc"), (char **) 0);
    506508      if (!indirect)

comment:5 follow-up: Changed 8 years ago by wjp

Did you forget to commit the changes in the spkg?

comment:6 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by jdemeyer

Replying to wjp:

Did you forget to commit the changes in the spkg?

Yes and no. I prefer to commit them after positive_review (this leaves a cleaner hg repository if reviewers request small changes).

comment:7 Changed 8 years ago by jdemeyer

  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. Little or no feedback.

comment:8 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:9 Changed 8 years ago by jdemeyer

Committed changes now.

comment:10 follow-up: Changed 8 years ago by wjp

I'm not sure how you triggered this error in sage (and therefore if the actual behaviour is really relevant), but are you sure this is the right behaviour when TERM is unset? I'd probably expect an unset TERM to behave the same as an empty TERM. Your suggested change prevents the TERMCAP value from being used at all in this case, I believe.

comment:11 in reply to: ↑ 10 Changed 8 years ago by jdemeyer

Replying to wjp:

I'm not sure how you triggered this error in sage (and therefore if the actual behaviour is really relevant)

There is more info at #11970. It's a combination of the new readline spkg at #11970 and the patch at #12263, and it also depends on the gcc version.

but are you sure this is the right behaviour when TERM is unset? I'd probably expect an unset TERM to behave the same as an empty TERM.

In practice, this will be the case. The tgetent() code checks whether the argument char *term equals the environment variable "TERM". Normally, term will not be the empty string. But you are certainly right that

term_name = getenv("TERM");
if (!term_name)
    term_name = "";

would also work.

comment:12 Changed 8 years ago by jdemeyer

  • Summary changed from Fix strcmp() will NULL argument in termcap library to Fix strcmp() with NULL argument in termcap library

comment:13 follow-ups: Changed 8 years ago by ppurka

Just a small comment: line 10 of Makefile.patch should say

+# I commented this (William), since SAGE install should work as not-root.

My experience with Makefiles is quite elementary, so I can not comment on Makefile.patch. Aside from that, the rest of the changes looks good.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 8 years ago by leif

Replying to ppurka:

Just a small comment: line 10 of Makefile.patch should say

+# I commented this (William), since SAGE install should work as not-root.

My experience with Makefiles is quite elementary, so I can not comment on Makefile.patch. Aside from that, the rest of the changes looks good.

Well, William's change(s) are more or less superfluous anyway, so I'd drop them. (I looked at the spkg a while ago, but the little changes weren't worth a ticket or new spkg.

  • Instead of changing the target all, we should use $MAKE libtermcap.a, then the info files won't get built, and hence not installed (also without commenting out the for loop).
  • Commenting out oldinstalldir=... isn't necessary, but also wrong; it should either be set to the empty string, or (if we assume nobody builds Sage as root) left as is, since the lines of the receipt that try to install into /usr/include are preceded by dashs anyway, i.e. make doesn't fail because of lacking file permissions.

In the latter case we wouldn't have to patch the Makefile at all.

(Slightly OT: Should we in general issue a warning -- or abort the build -- if someone builds Sage as root?)


Another minor thing: In spkg-install, -m64 should be appended, since it should take precedence over whatever a user specified in CFLAGS.

comment:15 in reply to: ↑ 14 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to leif:

Well, William's change(s) are more or less superfluous anyway, so I'd drop them. (I looked at the spkg a while ago, but the little changes weren't worth a ticket or new spkg.

  • Instead of changing the target all, we should use $MAKE libtermcap.a, then the info files won't get built, and hence not installed (also without commenting out the for loop).
  • Commenting out oldinstalldir=... isn't necessary, but also wrong; it should either be set to the empty string, or (if we assume nobody builds Sage as root) left as is, since the lines of the receipt that try to install into /usr/include are preceded by dashs anyway, i.e. make doesn't fail because of lacking file permissions.

In the latter case we wouldn't have to patch the Makefile at all.

Maybe you are right, but I don't want to make these minor changes now in this blocker ticket. If you want to make a follow-up ticket for the above, go ahead.

Another minor thing: In spkg-install, -m64 should be appended, since it should take precedence over whatever a user specified in CFLAGS.

Done.

comment:16 in reply to: ↑ 13 Changed 8 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Replying to ppurka:

Just a small comment: line 10 of Makefile.patch should say

+# I commented this (William), since SAGE install should work as not-root.

Thanks, I fixed this comment.

My experience with Makefiles is quite elementary, so I can not comment on Makefile.patch.

Luckily, there is no need to review the Makefile. This is an old patch which was directly applied to the src/ directory, instead of being applied by spkg-install. I restored the upstream sources and made an explicit patch. So the file patches/Makefile.patch doesn't need to be reviewed for this ticket.

comment:17 follow-up: Changed 8 years ago by leif

Replying to jdemeyer:

If the environment variable TERMCAP is set and TERM is not set, then calling tgetent() from the termcap library produces a Segmentation Fault because it calls strcmp() with a NULL argument. Sage hits this problem because of readline, see #11970.

Two further remarks:

  • TERMCAP is incidentally set by $SAGE_ROOT/spkg/install (like a couple of other "abused" environment variables).
  • readline isn't supposed to use Sage's termcap; the p4 at #11970 avoids its use under all circumstances (perhaps unless one manually adds -L$SAGE_LOCAL/lib/ to LDFLAGS or the like).

comment:18 in reply to: ↑ 17 ; follow-up: Changed 8 years ago by jdemeyer

Replying to leif:

Replying to jdemeyer:

If the environment variable TERMCAP is set and TERM is not set, then calling tgetent() from the termcap library produces a Segmentation Fault because it calls strcmp() with a NULL argument. Sage hits this problem because of readline, see #11970.

Two further remarks:

  • TERMCAP is incidentally set by $SAGE_ROOT/spkg/install (like a couple of other "abused" environment variables).

Very true. Luckily, this problem is restricted to the Sage installation scripts, and not to running Sage (which would be a much more serious problem).

  • readline isn't supposed to use Sage's termcap;

Why not? My guess is that we ship termcap precisely because readline might need it.

comment:19 in reply to: ↑ 18 ; follow-ups: Changed 8 years ago by leif

Replying to jdemeyer:

Replying to leif:

  • readline isn't supposed to use Sage's termcap;

Why not? My guess is that we ship termcap precisely because readline might need it.

No, a few other packages like PARI use it. If readline would use it, we should also build a shared library (which the package as is doesn't offer), and, more importantly, use that termcap library's headers (-I$SAGE_LOCAL/include).

But somebody meanwhile messed up the dependencies in spkg/standard/deps as if readline would depend on (Sage's) termcap.

In principle we could drop the spkg, since there are [newer] replacements on every supported system AFAIK.


One more thing: Changelog lacks an "e" in SPKG.txt.

I'd rename Makefile.patch to Makefile.in.patch to avoid confusion.

Changed 8 years ago by jdemeyer

comment:20 in reply to: ↑ 19 Changed 8 years ago by jdemeyer

Replying to leif:

One more thing: Changelog lacks an "e" in SPKG.txt.

I'd rename Makefile.patch to Makefile.in.patch to avoid confusion.

I made these changes, needs review.

comment:21 in reply to: ↑ 19 Changed 8 years ago by jdemeyer

Replying to leif:

But somebody meanwhile messed up the dependencies in spkg/standard/deps as if readline would depend on (Sage's) termcap.

Since #11970, readline does link in Sage's termcap (at least with some compilers/linkers), so I don't think that #12098 was a mistake.

comment:22 in reply to: ↑ 19 ; follow-ups: Changed 8 years ago by jdemeyer

Replying to leif:

No, a few other packages like PARI use it.

PARI certainly doesn't (I find no mention of the string "termcap" in the PARI source tree).

In principle we could drop the spkg, since there are [newer] replacements on every supported system AFAIK.

If this is true, then I would support this. I don't know whether your assumption ("replacements on every supported system") is true though.

comment:23 in reply to: ↑ 22 Changed 8 years ago by ppurka

Replying to jdemeyer:

Replying to leif:

In principle we could drop the spkg, since there are [newer] replacements on every supported system AFAIK.

If this is true, then I would support this. I don't know whether your assumption ("replacements on every supported system") is true though.

This might very well be true. I see this in the README of termcap:

However, use of termcap is discouraged.  Termcap is
being phased out in favor of the terminfo-based ncurses library, which
contains an emulation of the termcap library routines in addition to
an excellent curses implementation.  ncurses is available from the
usual GNU archive sites.

terminfo seems to be around since 1981. That's a long enough time for all systems to have caught up by now. Other websites: http://www.catb.org/~esr/terminfo/ http://en.wikipedia.org/wiki/Terminfo

comment:24 Changed 8 years ago by jhpalmieri

Just for fun I removed the termcap spkg and modified spkg/install and spkg/standard/deps accordingly. All tests passed on sage.math and on OS X 10.6. I haven't tried on any other platforms.

comment:25 in reply to: ↑ 22 Changed 8 years ago by leif

Replying to jdemeyer:

Replying to leif:

No, a few other packages like PARI use it.

PARI certainly doesn't (I find no mention of the string "termcap" in the PARI source tree).

Look into (e.g.) pari-*/src/config/get_readline.

PARI usually looks for readline, and then ncurses, then eventually gives libtermcap a try (for tgetent(); but apparently only in conjunction with readline).

comment:26 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Report Upstream changed from Reported upstream. Little or no feedback. to None of the above - read trac for reasoning.

comment:27 Changed 8 years ago by GeorgSWeber

  • Status changed from needs_review to positive_review

Looks good to me.

comment:28 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.8.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed

Many thanks for the review.

comment:29 Changed 8 years ago by jdemeyer

  • Reviewers set to Georg S. Weber

comment:30 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by kini

Replying to jdemeyer:

Replying to wjp:

Did you forget to commit the changes in the spkg?

Yes and no. I prefer to commit them after positive_review (this leaves a cleaner hg repository if reviewers request small changes).

FWIW, you can amend the latest commit in a Mercurial repository by doing the following: hg qimport -r tip, make some edits, hg qrefresh -De (to update the commit date/time and message), hg qfinish tip.

comment:31 in reply to: ↑ 30 ; follow-up: Changed 8 years ago by jdemeyer

Replying to kini:

FWIW, you can amend the latest commit in a Mercurial repository by doing the following: hg qimport -r tip, make some edits, hg qrefresh -De (to update the commit date/time and message), hg qfinish tip.

I didn't know this, thanks for the "tip".

comment:32 in reply to: ↑ 31 ; follow-up: Changed 8 years ago by leif

Replying to jdemeyer:

Replying to kini:

FWIW, you can amend the latest commit in a Mercurial repository by doing the following: hg qimport -r tip, make some edits, hg qrefresh -De (to update the commit date/time and message), hg qfinish tip.

I didn't know this, thanks for the "tip".

FWIW, for a single "uncommit", you can also use hg rollback.

comment:33 in reply to: ↑ 32 ; follow-up: Changed 8 years ago by kini

Replying to leif:

FWIW, for a single "uncommit", you can also use hg rollback.

Well, actually hg rollback will undo the latest action that was a commit, pull, unbundle, or any other modification to the data store. So that might not be the latest commit, in general. It might not even be under your control if you are allowing users to push to your repository remotely, as someone's push to your repo also counts as something to rollback. And it only stores one action to undo, so if any other such actions have been performed since the latest commit, you can no longer use hg rollback to uncommit it.

comment:34 in reply to: ↑ 33 Changed 8 years ago by leif

Replying to kini:

Replying to leif:

FWIW, for a single "uncommit", you can also use hg rollback.

Well, actually hg rollback will undo the latest action that was a commit, pull, unbundle, or any other modification to the data store. So that might not be the latest commit, in general. It might not even be under your control if you are allowing users to push to your repository remotely, as someone's push to your repo also counts as something to rollback. And it only stores one action to undo, so if any other such actions have been performed since the latest commit, you can no longer use hg rollback to uncommit it.

:-) As always, one should know what one's doing...

I was thinking of (and originally planned to write) smth like

while true; do
    hg commit -m "foo"
    hg rollback
    # edit
    # reupload spkg
done
Note: See TracTickets for help on using tickets.