回复: 回复: [8u] RFR(XS) JFR generate same symbol ID for different symbols (high probabily with CDS)(Internet mail)

[kalinshi(施慧)]

Hi All,

Please help review this JFR issue again in 8u. This problem can be easily reproduced with CDS and reproduce steps in JBS.

Fix includes:
1.  Remove map_symbol/map_cstring methods with only hashcode, all lookup must pass symbol or cstring with hashcode.
2.  In JfrSymbolId::equals methods, compare symbol reference or cstring contents when hashcodes are same.
3.  Rebuild anonymous klass cstring when search its sctring entry id.

JBS: https://bugs.openjdk.java.net/browse/JDK-8253837
Webrev: http://cr.openjdk.java.net/~hshi/8253837/webrev_2/

Regards
Hui

-----邮件原件-----
发件人: Andrew Dinn <adinn at redhat.com> 
发送时间: 2020年10月9日 20:57
收件人: kalinshi(施慧) <kalinshi at tencent.com>; jdk8u-dev at openjdk.java.net; Jaroslav Bachorík <jaroslav.bachorik at datadoghq.com>; Langer, Christoph <christoph.langer at sap.com>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
主题: Re: 回复: [8u] RFR(XS) JFR generate same symbol ID for different symbols (high probabily with CDS)(Internet mail)

Hi Hui,

On 09/10/2020 12:56, kalinshi(施慧) wrote:
> “I can see that it is possible that two different symbols might 
> generate the same hash, which is what appears to motivate your patch.
> However, you have not really explained in what circumstances that 
> problem will happen. That makes it impossible to know whether the 
> patch is really needed.“
> 
> This problem happens when running with CDS and reproduce steps is in 
> https://bugs.openjdk.java.net/browse/JDK-8253837.

Apologies for not seeing that explanation. When I opened the JIRA the description tab was not displayed. This now makes a lot more sense.

> " Also, if it is needed then it is important to know whether this is 
> the only thing that needs fixing or if more needs to be done. At the 
> very least I think you will need to patch both JfrSymbolId::equals 
> methods."
> 
> I have noticed " bool JfrSymbolId::equals(const char* query, uintptr_t 
> hash, const CStringEntry* entry) ". Plan is to investigate this after 
> finding a reproducible case, this code is quite different from JDK11. 
> In 8u, some caller is from JfrSymbolId::map_cstring(uintptr_t hash) 
> with NULL query and hash code, while in 11u map_cstring method has 
> removed and a declaration is left.

Well, I see what you mean regarding calls via map_cstring. There is no possibility of query and entry->literal() being different strings with the same hashcode when equals is called via map_cstring because it only calls lookup_only() with query == null.

However, the cstring table has to be updated somewhere and this happens when method id() is called in JfrSymbolId::mark. id() calls lookup_put() with a non null string. Now that looks to me like it can suffer the same problem as the symbol insert. If two symbols can generate the same hash then I don't see why two C strings could not also generate the same hash. That may be unlikely but I don't think it is impossible. So, I think it is going to be necessary to change both equals methods. Do you have any reasoning that explains why a hash collision is not possible?

I'd also still like to see a response from one of the upstream devs who worked on JDK-8231081 to explain why it needed to make allowance for hash collisions and also to ask if they see any good reason to backport JDK-8231081. If we need to do that anyway then we might as well apply all the required changes in one go. Jaroslav, any comments?

regards,


Andrew Dinn
-----------
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill