pgfkeys: Undefined .@cmd handler check is not robust

[Skillmon]

Brief outline of the bug

If the internal .@cmd-handler got defined to do nothing (or is \relax) a stray \pgfeov might end up in the input stream leading to a Undefined control sequence error.

Minimal working example (MWE)

\documentclass[]{article}

\usepackage{pgfkeys}

% just to make things trip
\csname pgfk@/foo/.@cmd\endcsname

\begin{document}
\pgfkeys{/foo}
\end{document}

---

Possible fix: Do \let\pgfeov\relax (I haven't thoroughly checked whether this might have side effects, but doubt it).

[hmenke]

This definition of /foo/.@cmd is obviously invalid. How did this situation occur?

[hmenke]

The underlying problem comes from Till's original definition of \pgfkeys@ifcsname. The pre-eTeX definition is not equivalent to \ifcsname, but when I switched pgfkeys to eTeX I was too naive and didn't think about this more. What's also interesting is that if you were always using an eTeX engine, this was broken since 2007.

pgf/generic/pgf/utilities/pgfkeys.code.tex

Lines 17 to 21 in 641a883

	\def\pgfkeys@ifcsname#1\endcsname#2\else#3\fi{\expandafter\ifx\csname#1\endcsname\relax#3\else#2\fi}%
	\ifx\eTeXrevision\undefined%
	\else%
	\expandafter\let\expandafter\pgfkeys@ifcsname\csname ifcsname\endcsname%
	\fi

My proposed patch is very simple. Just check \ifx\csname\relax instead of \ifcsname:

diff --git a/tex/generic/pgf/utilities/pgfkeys.code.tex b/tex/generic/pgf/utilities/pgfkeys.code.tex
index c0877e22..4a3d0791 100644
--- a/tex/generic/pgf/utilities/pgfkeys.code.tex
+++ b/tex/generic/pgf/utilities/pgfkeys.code.tex
@@ -200,10 +200,10 @@
 % \pgfkeysifdefined{/tikz/length}{key exists}{does not exist}
 
 \long\def\pgfkeysifdefined#1{%
-  \ifcsname pgfk@#1\endcsname
-    \expandafter\pgfkeys@firstoftwo
-  \else
+  \expandafter\ifx\csname pgfk@#1\endcsname\relax
     \expandafter\pgfkeys@secondoftwo
+  \else
+    \expandafter\pgfkeys@firstoftwo
   \fi
 }

With this patch you will get the expected

! Package pgfkeys Error: I do not know the key '/foo' and I am going to ignore 
it. Perhaps you misspelled it.

[Skillmon]

There is another, imho, more elegant option to fix this (the code is slightly slower, though):

\long\def\pgfkeysifdefined#1%
  {%
    \expandafter
    \ifx\csname\ifcsname pgfk@#1\endcsname pgfk@#1\else relax\endcsname\relax
      \expandafter\pgfkeys@secondoftwo
    \else
      \expandafter\pgfkeys@firstoftwo
    \fi
  }

This doesn't create a hash table entry if its unncessary.

As to how I stumbled upon this: Of course by playing with code and trying different things, accidentally creating a hash table entry for an undefined macro :)

[hmenke]

Thanks, I'll add this to my book of tricks, but for this to actually be useful in pgfkeys we'd have to refactor it to remove all the other unguarded uses of \csname as well.

[muzimuzhi]

This definition of /foo/.@cmd is obviously invalid. How did this situation occur?

As Henri mentioned, a /.@cmd subkey is always expected to be a macro whose arg-spec ends with \pgfeov so an empty arg-spec is invalid and will not exist if keys are defined by \pgfkeysdef (or its friends) and key handlers.

pgf/tex/generic/pgf/utilities/pgfkeys.code.tex

Lines 379 to 384 in 4cba1da

	\def\pgfkeys@case@one{%
	\pgfkeysifdefined{\pgfkeyscurrentkey/.@cmd}%
	{\pgfkeysgetvalue{\pgfkeyscurrentkey/.@cmd}{\pgfkeys@code}%
	\expandafter\pgfkeys@code\pgfkeyscurrentvalue\pgfeov}
	{\pgfkeys@case@two}%
	}

\documentclass{article}
\usepackage{pgfkeys}

% just to make things trip
% make sure its arg-spec ends with \pgfeov
\expandafter\def\csname pgfk@/foo/.@cmd\endcsname\pgfeov{}

\begin{document}
\pgfkeys{/foo}  % error since no default value is specified
\pgfkeys{/foo=} % ok

\pgfkeysifdefined{/foo/.@cmd}{true}{false} % leaves "true"
\end{document}

If a smarter error message is expected here, then what's needed is to check whether the arg-spec ends with \pgfeov, not the proposed #1132. I mean, accepting an empty arg-spec \pgfk@<path>/.@cmd worth little since there're all kinds of other invalid arg-specs that will raise similar mysterious error(s).

[Skillmon]

@muzimuzhi #1132 does not check for invalid arg specs and doesn't try to. Instead it restores old behaviour as Henri pointed out already (that was changed in 2007) in a more robust way. The thing is that the PR doesn't change anything about expected or planned behaviour but it allows to correctly detect undefined keys reliably.

[hmenke]

@muzimuzhi The problem at hand and the fix proposed in #1132 is to remove the asymmetry between \ifcsname and \csname. Consider:

\pgfkeysgetvalue{/foo/.@cmd}\foodef
\pgfkeys{/foo}

The first statement will let \foodef=\relax as documented and expected, but the second statement will blow up with undefined control sequence instead of unknown key.

EDIT: This is essentially what Skillmon just said.

[Skillmon]

Of course that's not the entire truth about the PR, it also makes changes to other usages of \csname such that pgfkeys doesn't create hash table entries when not necessary (something that's nice to have but not necessarily needed for the aforementioned detection).

[muzimuzhi]

@Skillmon @hmenke The proposed change will change something.

\pgfkeyslet{/bar}{\relax} % or \pgfkeys{/bar/.initial=\relax}
\pgfkeys{/bar}

• Before: passes
• After: raise an error I do not know the key '/bar' ...

With a wrongly defined \pgfk@/foo/.@cmd, it's hard to say how pgfkeys could do. Deciding /foo is defined is not so bad.

For the hash table entry thing, I checked all the use of \csname in pgfkeys.code.tex and find its use of \csname is all necessary and will not assign should-be-undefined thing to \relax. Hence checking by using \ifcsname suffices.