Fix emitter states handling when write_indicator fails

[perlpunk]

There are cases where yaml_emitter_write_indicator fails. In that case POP is called on emitter->indents but not on emitter->states, which results in a leftover event in the stack, and later POP is called on an empty emitter->indents stack.

This commit does not fix the case of the failing yaml_emitter_write_indicator. This is still investigated.

This will mitigate CVE-2024-3205

[perlpunk]

apparently some tests are failing, so there are cases where this is wrong... looking...

edit: fixed syntax issue

[zhuofeng6]

That's great. i think it is a more general fix for this. So can we merge it?

[Martchus]

I'm not familiar with the code but as far as I can see this makes sense.

[perlpunk]

Probably the emitter->state = POP(emitter, emitter->states); can be done directly after the POP on indents, instead of 3 times in different places.
I don't know why @xitology put it in that order. I can't imagine a case where it would be correct to only pop from indents.

[perlpunk]

I moved the POP now directly after the indents POP statement. That should have the same effect.

[ingydotnet]

I'm currently extremely busy but I'll try to review this soon.

[zhuofeng6]

This is a high score cve, which has a great impact on the libyaml community. If possible, it is necessary to merge this pr as soon as possible. This is my humble opinion

[frenzymadness]

The CVSS score for this vulnerability says that the attack complexity is low and the attract vector is the network (which means almost anybody can use this vulnerability to attack a system via a network). From the discussions here, I have a very different feeling. Even the maintainers of the software itself have a hard time exploiting/reproducing the vulnerability. It's either easy to reproduce or the score and therefore the severity of the vulnerability is wrong.

[perlpunk]

Note that the reproducers I found use the canonical mode, which is probably rarely used. But I think it could be possible to produce the same effect without canonical mode, and possibly someone already knows the necessary input for that.

[perlpunk]

Please see my update that I don't consider it a vulnerability: #258 (comment)

[zhuofeng6]

I agree with you. I don't think this is a cve either. but now does this PR still need to be merged?

[perlpunk]

I think the PR an improvement, but I would rather try to check in yaml_emitter_close if the emitter is in an error state. probably I can just check if an error was set.
just very busy in the middle of two conferences, and it's not urgent IMHO

[perlpunk]

I will create a different PR when I have time