ksh: Crash when running `set` in a subshell/comsub with discipline function defined

set outputs the values of all the variables. This crashes in the following reproducer:

function GIT_BRANCH.get
{
        command -v git >/dev/null || return
        .sh.value=${ git branch 2>/dev/null; }
        case $'\n'${.sh.value} in
        *$'\n* '*)
                .sh.value=$'\n'${.sh.value}
                .sh.value=${.sh.value#*$'\n* '}
                .sh.value=${.sh.value%%$'\n'*}
                ;;
        *)      .sh.value=''
                ;;
        esac
}
v=$(set)

About this issue

Original URL
State: closed
Created a year ago
Comments: 48

Commits related to this issue

Fix $_ corruption in subshell disc functs (re: 88a1f3d6, 0a343244) The _ variable is used for various context-dependent things and is sometimes a name reference (see man page under Variables). The $_... — committed to ksh93/ksh by McDutchie a year ago
Fix $_ corruption in subshell disc functs (re: 88a1f3d6, 430e4781) The _ variable is used for various context-dependent things and is sometimes a name reference (see man page under Variables). The $_... — committed to JohnoKing/ksh by McDutchie a year ago

Most upvoted comments

Tested successful as described in issue with patch 5. I also checked my memory fault notes and did not find anything else to add to the mix.

hyenias on Apr 3, 2023

Forgot to say, patch-five run like a champ on ubuntu 22.04 s390x as well.

phidebian on Apr 3, 2023

patch-five runs like a champ 😃 you are a star!

phidebian on Apr 3, 2023

Ha you beat me on this one, I learned how to monitor L_ARGNOD, thank you for that, and I discovered this.

print_scan()  loop until np->nvname=="Z" at this point L_ARGON is OK
   print_namval(np,...)
      nv_getval(np)
          nv_getv(np)
              lookups(np)
                   lookup(np...)

                     ====  Here may be the begining of problems... ====
  >407      block(bp,type);                                                  
   408      block(bp, UNASSIGN);   /* make sure nv_setdisc doesn't invalidate
   409      sh_pushcontext(&checkpoint, 1);                                  
   410      jmpval = sigsetjmp(checkpoint.buff, 0);       <==== setjmp No 1 here                   
   411      if(!jmpval)                                                      
   412         sh_fun(nq,np,NULL);                                      <==== will invoke Z.get()      
   413      sh_popcontext(&checkpoint);        

         
                   lookup(np) 
                        sh_fun(np)
                        ==== Second part of the problem 'may be' =====


  >3240   if(nq)                                                             
   3241     mode = set_instance(nq,&node, &nr);       <==== Save L_ARGNOD into &node                        
   3242   if(is_abuiltin(np))                                                
   3243   {                                                                  
 ......
   3266   }                                                                  
   3267   else                                                               
   3268     sh_funct(np,n,argv,NULL,sh_isstate(SH_ERREXIT));    <=== Here problem with sh_function do longjmp             
   3269   if(nq)                                                +=== to setjmp No1 so we never restore L_ARGNOD
   3270     unset_instance(nq, &node, &nr, mode);       <==== restore L_ARGNOD    NEVER reached if the above longjmp           
   3271   fcrestore(&save);

I am not fluent enough with the setjmp/longjmp pushcontext/popcontext, I dunno how they are linked together etc… but I think if you manage set a setjmp/longjmp point so on error in sh_funct() we longjmp back to the restore point for L_ARGNOD, you would be safe and no need update set_instance() unset_instance() unless theire are bugged.

phidebian on Apr 2, 2023

Well, dunno why I didn’t thought earlier, when I discovered that the name of the variable did matter, i.e Z is above _ and z is below, I feared that interaction with discipline messing with _ and print_nameval accessing _ could be problematic.

I decided to make a brutal kludge, i.e skipping over _ in print_scan (src/cmd/ksh93/bltins/typeset.c)

		if((np=nv_search(*argv++,root,0)) && np!=onp && (!nv_isnull(np) || np->nvfun || nv_isattr(np,~NV_NOFREE)))
		{
			onp = np;
                       /* kludge start */
                        if( (np->nvname[0]=='_')&&(np->nvname[1]==0) )  
                        { continue;
                        }
                        /* kludge end */

And to my grand surprise, no more core dump, and no regression in QA, even though now b=$(set) miss the _ variable, IMHO this is not a big lost and ledgitmate to withdraw, as we withdraw .sh etc… the value of _ at set time will be overloaded right after b=$(set) as _=$(set)

Anyway this means that may be all the things with fork() is still good, only the _ access in print_scan is not good, I noticed that when not doing the kludge, i.e that actual prod path, then effectivly np pointer got corrupted data leading to crashdump.

Is that a trail to follow ?

EDIT: Well now I got no core dump, but later echo $_ do core dump, so _ is corrupted anyway, simply not being printed in the print_scan() differ it. So may be fork() is back 😃

phidebian on Apr 2, 2023