Skip to content

Minor Issues Found in fopen() patch #349

@spe-ciellt

Description

@spe-ciellt

Minor Issues Found in fopen() patch

1. logToFileFilename still uses raw fopen()

main.c:950:

logFile = fopen(logToFileFilename, "w");

logToFileFilename is set from optarg at main.c:824 (the --log option), which comes from argv. After the PR's argv UTF-8 conversion those strings are in UTF-8, but fopen() on Windows still interprets them as system-codepage. A user who passes --log /path/with/ünïcödé/log.txt would get a silent open failure even with this PR applied. This should be g_fopen().

If this call were converted, the #include <glib/gstdio.h> added to main.c would be justified (currently the include is not needed — see issue 2).

2. #include <glib/gstdio.h> in main.c is premature

The PR adds <glib/gstdio.h> to main.c, but main.c does not call g_fopen() anywhere in the patched version. g_locale_to_utf8() is declared in <glib.h>, which is already pulled in transitively. The include is harmless but signals intent that wasn't followed through — see issue 1.

3. #ifdef G_OS_WIN32 vs. #ifdef WIN32 inconsistency

The new argv conversion block uses G_OS_WIN32:

#ifdef G_OS_WIN32
    for (i = 0; i < argc; i++) { … }
#endif

The surrounding code in main.c consistently uses WIN32:

# ifdef WIN32
    bind_textdomain_codeset(PACKAGE, "UTF-8");
# endif
#ifdef WIN32
    screenRenderInfo.renderType = GERBV_RENDER_TYPE_CAIRO_NORMAL;
#endif

Both macros resolve on Windows in this build (GLib defines G_OS_WIN32; WIN32 is compiler-defined), so there is no functional difference. Cosmetic inconsistency only — the new block should use #ifdef WIN32 to match the file's existing style.

4. argv mutation produces a Valgrind-visible leak (by design)

gchar *utf8_arg = g_locale_to_utf8(argv[i], -1, NULL, NULL, NULL);
if (utf8_arg) {
    argv[i] = utf8_arg;
}

The original argv[i] pointer (owned by the C runtime) is overwritten and cannot be freed. The new utf8_arg allocation is placed into argv[] and also never freed. This is intentional — these strings must live for the process lifetime — but Valgrind would report them as definite leaks. Since the G_OS_WIN32 guard means this block never runs on Linux (where the Valgrind tests run), there is no practical impact on the test suite. Worth noting in a code comment.

No show stoppers, I merge this with referencing this in an issue for future update.

Originally posted by @spe-ciellt in #318 (comment)

Metadata

Metadata

Assignees

No one assigned

    Labels

    fixSolution for a potential problem or omission.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions