4
\$\begingroup\$

I wrote this code for opening URLs using C.

Is using enum for function result a good practice?

I also thought about sanitizing the URL, but it didn't seem easy and is. Probably, not a part of this function's responsibility.

#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
#include <stdio.h>

static char command[] = "start \"\" \"%s\"";
static size_t commandSize = sizeof(command) - 3ULL;

static size_t calculateCommandBufferSize(char const * url) {
    size_t urlSize = strlen(url);
    return urlSize + commandSize + 1;
}

/**
 * @brief describes possible results of `openURL`.
 */
typedef enum OPEN_URL_RESULT {
    OPEN_URL_SUCCESS,
    OPEN_URL_NULL_POINTER,
    OPEN_URL_OUT_OF_MEMORY,
    OPEN_URL_COMMAND_BUFFER_FAIL,
    OPEN_URL_CMD_FAIL
} OPEN_URL_RESULT;

typedef struct OpenURLResult {
    OPEN_URL_RESULT result;
    int exit_code;
} OpenURLResult;

#define return_with(enum_result) return (OpenURLResult){enum_result, 0};

/**
 * @brief execute command for opening url in the default browser using Windows
 * command line.
 * @warning Do not accept url as input from user without sanitizing it as the
 * program is prone to injections
 * 
 * @param url url to be opened
 * @return true if executed successfully
 * @return false if failed to execute
 */
OpenURLResult openURL(char const * url) {
    if (url == NULL) {
        return_with(OPEN_URL_NULL_POINTER);
    }

    size_t const buffer_size = calculateCommandBufferSize(url);
    char *buffer = malloc(buffer_size * sizeof(char));
    if (buffer == NULL) {
        return_with(OPEN_URL_OUT_OF_MEMORY);
    }
    int const print_result = sprintf(buffer, command, url);
    if (print_result < 0) {
        return_with(OPEN_URL_COMMAND_BUFFER_FAIL);
    }

    int const exit_code = system(buffer);
    free(buffer);
    if (!exit_code) {
        return_with(OPEN_URL_SUCCESS);
    }
    OpenURLResult result = {OPEN_URL_CMD_FAIL, exit_code};
    return result;
}

#undef return_with
\$\endgroup\$
3
  • 3
    \$\begingroup\$ Incorporating advice from an answer into the question violates the question-and-answer nature of this site. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so the answers make sense again. \$\endgroup\$ Commented Oct 14, 2024 at 11:01
  • 1
    \$\begingroup\$ "Sanitizing the URL is probably not a part of this function's responsibility" - I would disagree heavily with that. Your function is receiving an URL, not a command, it's your function that is putting it inside a shell command, and therefore it is your responsibility to escape it appropriately in that very place. Even if you document it explicitly in that comment, this is a very error-prone design. \$\endgroup\$ Commented Oct 14, 2024 at 22:11
  • \$\begingroup\$ Since you are platform specific anyway, one suggestion would be to actually execute the user's browser choice (by inspecting the registry) instead of using the system crutch. It should be more efficient. \$\endgroup\$ Commented Oct 15, 2024 at 2:16

1 Answer 1

Reset to default
3
\$\begingroup\$

I tried adding a simple main() function:

int main()
{
    return openURL("https://codereview.stackexchange.com").exit_code;
}

However, there seems to be a dependency on an external program, as when I run this it just gives an error:

sh: 1: start: not found

You should add some notes, perhaps listing packages that need to be installed, at the start of the program. Otherwise, the code is of very little use.

As an aside, this failure results in a success return code from the program, because we return the result of system() directly (which on my system truncates 0x7f00, yielding 0), rather than unpacking it with WEXITSTATUS() or just converting any non-zero return into EXIT_FAILURE. So we should be clearer about the interpretation of the return value, so that I know I must write

int main()
{
    return openURL("https://codereview.stackexchange.com").exit_code
        ? EXIT_FAILURE : EXIT_SUCCESS;
}

I see no need to include <stdbool.h>.

buffer_size * sizeof(char) can be re-written as buffer_size * 1 or more simply as buffer_size.

The significance of the value 3ull in sizeof(command) - 3ULL should be explained. And I see no reason for the parentheses around command - that just makes it look like you're applying sizeof to a type name.

Probably better to use snprintf() than sprintf() so that any changes that affect the length computation can be detected and corrected.

\$\endgroup\$
6
  • \$\begingroup\$ are you on windows? The code says that the url is opened though Windows command line \$\endgroup\$ Commented Oct 14, 2024 at 8:30
  • \$\begingroup\$ Oh, the question isn't tagged for Windows, so I didn't realise it was intended to be platform-specific. Are Windows system() returns directly usable as return values from main(), then? \$\endgroup\$ Commented Oct 14, 2024 at 8:33
  • \$\begingroup\$ As far as I can tell, yes. I created 2 c files - one that returns 1, another one that takes compiled version of the first one and does printf("%i", system("first-one")), which indeed prints 1 \$\endgroup\$ Commented Oct 14, 2024 at 8:47
  • \$\begingroup\$ Though, when I did start "" "non-existent"; echo %ErrorLevel% it returned 9007, but the program, that uses the same command has exit code 1, that's strange. I should probably try WEXITSTATUS \$\endgroup\$ Commented Oct 14, 2024 at 8:52
  • \$\begingroup\$ WEXITSTATUS is POSIX, so might not apply on your platform. \$\endgroup\$ Commented Oct 14, 2024 at 9:22

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.