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

Add casr-lua #231

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Add casr-lua #231

wants to merge 15 commits into from

Conversation

hkctkuy
Copy link
Member

@hkctkuy hkctkuy commented Nov 29, 2024

No description provided.

@hkctkuy hkctkuy linked an issue Nov 29, 2024 that may be closed by this pull request
@SweetVishnya
Copy link
Collaborator

@hkctkuy, can you, also, please add casr-lua calls into casr-libfuzzzer. Thus, output from luzzer could be triaged.

@SweetVishnya
Copy link
Collaborator

@ligurio, can you help testing Lua crash analysis added in this PR?

@ligurio
Copy link

ligurio commented Nov 30, 2024

@SweetVishnya Sure, will do after weekend.

Copy link

codecov bot commented Nov 30, 2024

Codecov Report

Attention: Patch coverage is 81.53310% with 53 lines in your changes missing coverage. Please review.

Project coverage is 66.38%. Comparing base (af9e9e4) to head (0171139).

Files with missing lines Patch % Lines
libcasr/src/lua.rs 81.74% 23 Missing ⚠️
casr/src/bin/casr-lua.rs 88.00% 15 Missing ⚠️
casr/src/bin/casr-cli.rs 0.00% 12 Missing ⚠️
libcasr/src/report.rs 75.00% 2 Missing ⚠️
libcasr/src/python.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #231      +/-   ##
==========================================
+ Coverage   65.87%   66.38%   +0.51%     
==========================================
  Files          32       34       +2     
  Lines        7879     8154     +275     
==========================================
+ Hits         5190     5413     +223     
- Misses       2689     2741      +52     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@ligurio ligurio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch!
In general, it works, but with limitations. Seem comments inline.


// Extract lua exception
let Some(exception) = LuaException::new(&stderr) else {
bail!("Lua exception is not found!");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imagine a Lua code like the following:

[0] ~/sources/casr $ cat exc6.lua 
#!/bin/env lua

function a()
  error("Boom!", 3); 
end

function b() a(); end

function c() b(); end

function d() c(); end

function e() d(); end

function f() e(); end

function errhandler(msg)
  print (msg .. "\nstack traceback: \n" .. debug.traceback(2, 3, 2));
end

xpcall(f, errhandler);

that produces an output:

[0] ~/sources/casr $ ./exc6.lua 
./exc6.lua:9: Boom!
stack traceback: 
2
stack traceback:
        ./exc6.lua:4: in function 'a'
        ./exc6.lua:7: in function 'b'
        ./exc6.lua:9: in function 'c'
        ./exc6.lua:11: in function 'd'
        ./exc6.lua:13: in function 'e'
        ./exc6.lua:15: in function <./exc6.lua:15>
        [C]: in function 'xpcall'
        ./exc6.lua:21: in main chunk
        [C]: in ?

casr-lua will not find traceback there:

$ ./target/debug/casr-lua -o lua.casrep -- ./exc6.lua 
Error: Lua exception is not found!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error occurs only due to extra spaces after traceback

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can think about storing custom (maybe multiline) error as Explanation or Description fields, but it's not clear how to extract ShortDescription or etc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


// Extract lua exception
let Some(exception) = LuaException::new(&stderr) else {
bail!("Lua exception is not found!");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lua code:

#!/bin/env lua

local function f1()
  error('error is triggered', 1)
end

local function f2()
  f1()
end

local function f3()
  f2()
end

local function f4()
  f3()
end

local function f5()
  f4()
end

local thread = coroutine.create(f5)
local executed_ok, message = coroutine.resume(thread)

if not executed_ok then
  print("Error: "..message.." in ".. debug.traceback(thread))
end
[0] ~/sources/casr $ lua exc2.lua 
Error: exc2.lua:4: error is triggered in stack traceback:
        [C]: in function 'error'
        exc2.lua:4: in function 'f1'
        exc2.lua:8: in function 'f2'
        exc2.lua:12: in function 'f3'
        exc2.lua:16: in function 'f4'
        exc2.lua:20: in function <exc2.lua:19>
[0] ~/sources/casr $ ./target/debug/casr-lua -o lua.casrep -- ./exc2.lua 
Error: Lua exception is not found!
[0] ~/sources/casr $ 

Should it be supported?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... I assumed there was always going to be something like [C] in ? or [C] at 0xdeadbeaf at the end of traceback, but apparently it's not

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait... That's just debug info in stdout, not error message (although it contains word Error)
Is it possible to rewrite this logging message to stderr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second issue is multiple call of debug.traceback()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To sum up the above, I think we should not to support it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
Custom error messages are not something that we want to support


// Extract lua exception
let Some(exception) = LuaException::new(&stderr) else {
bail!("Lua exception is not found!");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stack trace for assertions does not work:

[1] ~/sources/casr $ cat exc7.lua 
#!/bin/env lua

assert(nil, 'dddd')
[1] ~/sources/casr $ lua exc7.lua 
lua: exc7.lua:3: dddd
stack traceback:
        [C]: in function 'assert'
        exc7.lua:3: in main chunk
        [C]: in ?
[1] ~/sources/casr $ ./target/debug/casr-lua -o lua.casrep -- ./exc7.lua 
(empty)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idk, It works fine for me

@hkctkuy hkctkuy requested a review from ligurio December 2, 2024 19:49
})
}
/// Extract stack trace from lua exception.
pub fn extract_stacktrace(&self) -> Result<Vec<String>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For stacktrace and exception we have different interfaces. Why is this code here?

Copy link
Member Author

@hkctkuy hkctkuy Dec 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The word exception in the interaface name can create confusion.
I just want to create additional layer of abstruction like UbsanWarning and use resulting interface for safe work with all necessary methods from ParseStacktrace, CrashLineExt and Severity.
Maybe it's worth to renaming it to LuaError or etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should handle Lua errors the same way we handle errors in other supported languages. We should implement the Exception trait for LuaError like here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's already implement the Severity trait

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main idea is transfer more parse logic to lib for having single entry point like for UbsanWarning.
In result you just give stream to lib and recieve all necessary info.

libcasr/src/lua.rs Outdated Show resolved Hide resolved
libcasr/src/lua.rs Outdated Show resolved Hide resolved
libcasr/src/lua.rs Outdated Show resolved Hide resolved
@hkctkuy hkctkuy requested a review from Avgor46 December 8, 2024 15:10
// Go
if let Ok(raw) = GoStacktrace::extract_stacktrace(&s) {
if let Ok(st) = GoStacktrace::parse_stacktrace(&raw) {
// C#
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix match above:

match data[0] % 8

@@ -30,17 +31,17 @@ fuzz_target!(|data: &[u8]| {
}
}
1 => {
// Go
if let Ok(raw) = GoStacktrace::extract_stacktrace(&s) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you chenge numbering here, you should also change corpus files

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

Successfully merging this pull request may close these issues.

Support Lua programming language
4 participants