Fix errno clobbered by cleanup in gerb_fopen()#319
Conversation
Callers of gerb_fopen() use strerror(errno) to report why a file failed to open, but the cleanup calls inside gerb_fopen() (fclose, g_free, munmap) could clobber errno before it was returned to the caller, producing misleading error messages. Save errno immediately after the failing call and restore it after cleanup, before returning NULL. This affects all six error paths: g_fopen failure, fstat failure, mmap failure, g_malloc failure, calloc failure, and fread failure. Also fixes a latent NULL pointer dereference on the mmap failure path, which previously set fd = NULL and fell through to fd->filename = g_strdup(...) instead of returning. Ref: gerbv#302
|
GLib's char *buf = (char *)g_malloc(fd->datalen + 1);
if (buf == NULL) { /* ← can never be true */
int saved_errno = errno;
munmap(fd->data, fd->datalen);
fclose(fd->fd);
g_free(fd);
errno = saved_errno;
return NULL;
}are unreachable. The PR did not introduce the dead Similarly, Considered minor and save for future updates. |
Summary
errnoacross cleanup calls (fclose,g_free,munmap) in all error paths ofgerb_fopen()mmapfailure pathFound while reviewing #302 — the
strerror(errno)pattern used by all three callers (gerbv_open_image,gerbv_create_rs274x_image_from_filename,gerbv_create_excellon_image_from_filename) relies ongerb_fopen()preservingerrno, but the cleanup calls beforereturn NULLcould clobber it.Details
gerb_fopen()has six error return paths after the initial allocation. In each case, cleanup functions (fclose(),g_free(),munmap()) run between the failing call andreturn NULL. These cleanup functions are allowed to modifyerrno, so callers that usestrerror(errno)may get an unrelated error message.The fix uses the standard POSIX
saved_errnoidiom — saveerrnoimmediately after the failing call, perform cleanup, then restore it before returning. This is the established pattern in GLib/POSIX C code (used throughout glibc internals, libgit2, curl, and GLib itself) for any wrapper function that performs cleanup between a failing syscall and its return.Additionally, the
mmapfailure path previously setfd = NULLand fell through tofd->filename = g_strdup(...), which would dereference NULL. This is now a properreturn NULL.Affected error paths
g_fopen()g_free()fstat()fclose(),g_free()mmap()fclose(),g_free()g_malloc()munmap(),fclose(),g_free()calloc()fclose(),g_free()fread()fclose(),g_free()x2Test plan
Ref: #302