Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sandbox 是否存在当 执行到spyMethodOnReturn 之后 当Ret.state==1 的时候栈未清空的情况,麻烦帮忙确认。 #328

Closed
xiaoxiaocaiiao opened this issue Jun 19, 2021 · 19 comments
Assignees
Labels
Milestone

Comments

@xiaoxiaocaiiao
Copy link

复现step

  1. 采用 -javaagent 方式启动 sandbox 增强 某个 方法 a
  2. 重新写一个 java agent 在 transfer 中采用 javassist 中的 hookMethod.insertAfter 修改 同一个方法 a 抛出异常 jdk.internal.org.objectweb.asm.tree.analysis.AnalyzerException: Error at instruction 125: Incompatible stack heights
    at jdk.internal.org.objectweb.asm.tree.analysis.Analyzer.analyze(Analyzer.java:324)
    这种情况 大多数因为try catch 引起
@xiaoxiaocaiiao
Copy link
Author

xiaoxiaocaiiao commented Jul 4, 2021

问题应该是出在了 如下代码,如果是的 这个问题会不会造成内存溢出 先抛开我这个问题不谈 ,sandbox是不是有 栈 没有清空 的危险,在 131行栈里面还有两个值 可是只pop 了一个 要清理最早的一个需要交换两个栈顶值后再pop 一次 否则会有栈未清空的可能,难道你们之前用的areturn 会清理最后的所有栈中值吗? 请帮忙确认这个问题。。
98: invokeinterface #163, 3
103: iconst_2
104: pop
105: dup
106: ldc_w #799 // String default
109: sipush 1005
112: invokestatic #822 // Method java/com/alibaba/jvm/sandbox/spy/Spy.spyMethodOnReturn:(Ljava/lang/Object;Ljava/lang/String;I)Ljava/com/alibaba/jvm/sandbox/spy/Spy$Ret;
115: dup
116: getfield #815 // Field java/com/alibaba/jvm/sandbox/spy/Spy$Ret.state:I
119: dup
120: iconst_1
121: if_icmpeq 131
124: iconst_2
125: if_icmpeq 141
128: goto 148
131: pop
132: getfield #818 // Field java/com/alibaba/jvm/sandbox/spy/Spy$Ret.respond:Ljava/lang/Object;
135: goto 205

@xiaoxiaocaiiao xiaoxiaocaiiao changed the title 先使用sandbox 修改 一个方法,再使用 javassist 中的 hookMethod.insertAfter 修改同一个方法,重新加载类, 类验证失败 报 栈平衡异常 sandbox 是否存在当 执行到spyMethodOnReturn 之后 当Ret.state==1 的时候栈未清空的情况,麻烦帮忙确认。 Jul 4, 2021
@z529192557
Copy link
Collaborator

z529192557 commented Jul 27, 2021

131执行完后,栈中只有Spy$Ret,在132中会用上, 131执行完后栈里应该是只有一个值

@z529192557
Copy link
Collaborator

最好把完整方法的字节码全部贴出来分析

@xiaoxiaocaiiao
Copy link
Author

这个问题我已经在自己的代码修复掉了,这里131 还有一个 object 的,只是sanbox不容易发现,因为 sandbox 在135 行用的return 之类的指令 栈会被清空 所以不会看出问题,但是一旦 出现多个 agent 同时 注入一个方法的时候就会出现 ,这里会有 类检查 会报栈平衡问题错误。

@xiaoxiaocaiiao
Copy link
Author

我贴的这个就已经可以看出问题了。

@xiaoxiaocaiiao
Copy link
Author

我就是通过这段代码分析出来的

@z529192557
Copy link
Collaborator

我明白你的意思了,栈底状态是 [Ret.respond, object],对吧,object 98行真实调用返回的

@xiaoxiaocaiiao
Copy link
Author

是的

@xiaoxiaocaiiao
Copy link
Author

需要把object 的 给pop 掉

@z529192557
Copy link
Collaborator

需要在ARETURN 的合适位置,swap pop一下,有时间的话,可以提交一份修复的PR吗

@xiaoxiaocaiiao
Copy link
Author

我可以把修复代码贴出来哈

@z529192557
Copy link
Collaborator

我可以把修复代码贴出来哈

感谢

@xiaoxiaocaiiao
Copy link
Author

xiaoxiaocaiiao commented Jul 27, 2021

我的修复点在 public class EventWeaver extends ClassVisitor implements Opcodes, AsmTypes, AsmMethods 中 的
return new ReWriteMethod下。

1.添加两个个新方法
private boolean isNullReturn(int opcode) {
return opcode == RETURN;
}
/**
* 流程控制
*/
private void processControlAfter() {
final Label finishLabel = new Label();
final Label returnLabel = new Label();
final Label throwsLabel = new Label();
dup();
visitFieldInsn(GETFIELD, ASM_TYPE_SPY_RET, "state", ASM_TYPE_INT);
dup();
push(Spy.Ret.RET_STATE_RETURN);
ifICmp(EQ, returnLabel);
push(Spy.Ret.RET_STATE_THROWS);
ifICmp(EQ, throwsLabel);
goTo(finishLabel);
mark(returnLabel);
pop();
swap();
pop();
visitFieldInsn(GETFIELD, ASM_TYPE_SPY_RET, "respond", ASM_TYPE_OBJECT);
checkCastReturn(Type.getReturnType(desc));
goTo(finishLabel);
mark(throwsLabel);
visitFieldInsn(GETFIELD, ASM_TYPE_SPY_RET, "respond", ASM_TYPE_OBJECT);
checkCast(ASM_TYPE_THROWABLE);
throwException();
mark(finishLabel);
pop();
}

2.修改一个 方法
@OverRide
protected void onMethodExit(final int opcode) {
if (!isThrow(opcode)) {
if(!isNullReturn(opcode)){
codeLockForTracing.lock(new CodeLock.Block() {
@OverRide
public void code() {
loadReturn(opcode);
push(namespace);
push(listenerId);
invokeStatic(ASM_TYPE_SPY, ASM_METHOD_Spy$spyMethodOnReturn);
processControlAfter();
}
});
}else {
codeLockForTracing.lock(new CodeLock.Block() {
@OverRide
public void code() {
loadReturn(opcode);
push(namespace);
push(listenerId);
invokeStatic(ASM_TYPE_SPY, ASM_METHOD_Spy$spyMethodOnReturn);
processControl();
}
});
}

            }
        }

@xiaoxiaocaiiao
Copy link
Author

这里面 有 构造器的问题所以 只有 有返回值需要多pop一次

@z529192557
Copy link
Collaborator

z529192557 commented Jul 27, 2021

这里有些不严谨的地方,我再看看,解决ARETURN是够了, 如果是LRETURN或DRETURN,似乎应该 用POP2,会更加复杂一些

@xiaoxiaocaiiao
Copy link
Author

关键是看 [Ret.respond, object] 中的 object 是什么类型,因为我们要移除掉的是object。

@xiaoxiaocaiiao
Copy link
Author

似乎如果出现了 lreturn 和drentrun swap 是不是也会有问题,具体我没有调查过。我当时想的比较好的办法是定义变量来 存 返回值 需要的时候取出来 用完就释放掉,这样是最靠谱的做法。只是当时为了尽快解决就修复了areturn 的。

@oldmanpushcart
Copy link
Collaborator

@xiaoxiaocaiiao hello,可以提一个PR么?我们merge进去

@oldmanpushcart
Copy link
Collaborator

已发布1.4.0,多谢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants