On several tasks if you pass a directory as a file to ssh-keygen, the program wouldn't check the file mode to check if it's a directory like: `Saving key "./test/" failed: Is a directory` After asking the user to overwrite or not. The file mode is already readed when getting `stat` Do you think checking it is a good idea? I've done something like:>From 2794c45c84c06999d977d44b69e9fc34e93c8336 Mon Sep 17 00:00:00 2001From: Salar Nosrati-Ershad <snosratiershad at gmail.com> Date: Mon, 6 Jan 2025 12:59:22 +0330 Subject: [PATCH] ssh-keygen: feat: raise error on confirm_overwrite if file is directory --- ssh-keygen.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ssh-keygen.c b/ssh-keygen.c index 89c3ed287..94665b0ab 100644 --- a/ssh-keygen.c +++ b/ssh-keygen.c @@ -242,6 +242,10 @@ confirm_overwrite(const char *filename) if (stat(filename, &st) != 0) return 1; + if (S_ISDIR(st.st_mode)) { + error("%s is a directory.", filename); + return 0; + } printf("%s already exists.\n", filename); printf("Overwrite (y/n)? "); fflush(stdout); -- 2.43.0
Salar Nosrati-Ershad wrote:> On several tasks if you pass a directory as a file to ssh-keygen, the > program wouldn't check the file mode to check if it's a directory like: > `Saving key "./test/" failed: Is a directory` > After asking the user to overwrite or not.Seems an appropriate error message is emitted to the caller if the argument is of an incorrect type. It's good. Right?> The file mode is already readed when getting `stat` > Do you think checking it is a good idea?My opinion is that this is not a good thing to do. Going down that route eventually leads to a lot of unnecessary code bloat chasing all of the types of anything that is not a file. There is already an appropriate error message printed. Why isn't that good enough? Why is the caller passing a directory intead of a file? Just as a discussion point, what if someone else comes along and says, hey if someone passes a Unix domain socket path to the program and asks if there should be an explicit test for that type? And then someone else says what if someone passes a block device? And then someone what about a character device? And what if... You get the idea. This type of chase just keeps chasing different types. The message saying that the save failed and the error as to why feels perfect to me and I have no idea why that would not be understood already. Bob