serenity icon indicating copy to clipboard operation
serenity copied to clipboard

mktemp: Make sure to use target directory option argument

Open kemzeb opened this issue 1 year ago • 5 comments

Resolves #22773.

Previously, we ignored the -p argument if it was specified. This would resort in a crash because final_target_directory wasn't given a value.

This snapshot does away with giving this variable an Optional<> and just has the -p argument be its default value.

kemzeb avatar Jan 15 '24 20:01 kemzeb

The only thing that's weird is what line the crash refers to (the following is the printout from the issue):

54.956 mktemp(50): VERIFICATION FAILED: m_has_value at ././AK/Optional.h:215
55.271 CrashReporter(51:52): --- Backtrace for thread #0 (TID 50) ---
55.271 CrashReporter(51:52): 0x0000000fb51a1db8: [/usr/lib/libc.so] ak_verification_failed +0x58 (Assertions.cpp:108 => Assertions.cpp:101)
55.271 CrashReporter(51:52): 0x000000080bdd5c2f: [/bin/mktemp] serenity_main(Main::Arguments) +0x55f (Optional.h:215 => Optional.h:234 => Optional.h:161 => mktemp.cpp:80)
55.271 CrashReporter(51:52): 0x000000080bdd5457: [/bin/mktemp] main +0x147 (Main.cpp:43)
55.271 CrashReporter(51:52): 0x000000080bdd55b4: [/bin/mktemp] _entry +0x24 (crt0.cpp:48)

I'm not sure why the crash is referring to this line: https://github.com/SerenityOS/serenity/blob/fbde901614368dcf03d4a8eee800d8b89131465f/Userland/Utilities/mktemp.cpp#L80C63-L80C63

When I run mktemp -p ., file_template should be empty at this point in execution so not sure why this is?

kemzeb avatar Jan 15 '24 20:01 kemzeb

I'm not sure why the crash is referring to this line: fbde901/Userland/Utilities/mktemp.cpp#L80C63-L80C63

When I run mktemp -p ., file_template should be empty at this point in execution so not sure why this is?

The stack trace is not always correct. Compiler optimisations can merge code, so the same output is used for multiple different sections of the code. Any stack traces pointing at any of those sections will instead point at the same one that they got merged into.

If you disable optimisations, that won't happen and you'll get the correct stack trace. Find add_compile_options(-O2) in the build files and change that to -O0 or -Oz temporarily.

AtkinsSJ avatar Jan 15 '24 21:01 AtkinsSJ

Sorry, I didn't see your PR before I made mine! I don't think this correctly handles the case where -p is used and the template is a directory. For example: mktemp -p /home anon/test.XXXXXX.

That said, I think that case is probably outside the scope of the issue being fixed, so I'd be happy to wait for this to be merged and update the description of mine to better describe what my changes do.

tcl3 avatar Jan 15 '24 21:01 tcl3

No problem! Yes, this edge case was something that I noticed too while testing both on my OS and in Serenity (this problems seem to have been caused by d9ff37e1b1c4388325f03ca924ad4848e0e62b67) but decided to get this PR merged before trying to tackle it. Not sure how to handle this conundrum, so I'll leave it up to the maintainers to decide how this should work out :^)

kemzeb avatar Jan 15 '24 21:01 kemzeb

Recent change rebases and resolves a merge conflict

kemzeb avatar Jan 16 '24 18:01 kemzeb