Bug 209078 - Minor bugs in vidcontrol
Summary: Minor bugs in vidcontrol
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: misc (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Ed Maste
URL: https://reviews.freebsd.org/D6178
Keywords: patch, vt
Depends on:
Blocks: 203349
  Show dependency treegraph
 
Reported: 2016-04-26 19:05 UTC by CTurt
Modified: 2017-02-10 15:03 UTC (History)
3 users (show)

See Also:
emaste: mfc-stable9-
emaste: mfc-stable10+
emaste: mfc-stable11+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2016-04-26 19:05:44 UTC
There is a memory leak in the `vidcontrol` utility in the `load_vt4font`:

usr.sbin/vidcontrol/vidcontrol.c:

static int
load_vt4font(FILE *f)
{
	struct vt4font_header fh;
	static vfnt_t vfnt;
	size_t glyphsize;
	unsigned int i;

	if (fread(&fh, sizeof fh, 1, f) != 1) {
		perror("file_header");
		return (1);
	}

	if (memcmp(fh.magic, "VFNT0002", 8) != 0) {
		fprintf(stderr, "Bad magic\n");
		return (1);
	}

	for (i = 0; i < VFNT_MAPS; i++)
		vfnt.map_count[i] = be32toh(fh.map_count[i]);
	vfnt.glyph_count = be32toh(fh.glyph_count);
	vfnt.width = fh.width;
	vfnt.height = fh.height;

	glyphsize = howmany(vfnt.width, 8) * vfnt.height * vfnt.glyph_count;
	vfnt.glyphs = malloc(glyphsize);

	if (fread(vfnt.glyphs, glyphsize, 1, f) != 1) {
		perror("glyphs");
		return (1);
	}

	for (i = 0; i < VFNT_MAPS; i++)
		vfnt.map[i] = load_vt4mappingtable(vfnt.map_count[i], f);

	if (ioctl(STDIN_FILENO, PIO_VFONT, &vfnt) == -1) {
		perror("PIO_VFONT");
		return (1);
	}
	return (0);
}

After the `vfnt.glyphs` buffer has been allocated with `malloc`, the function can return without freeing the buffer if `fread` or `ioctl` fail.

This is only a minor bug, since the process exits almost immediately after calling this function anyway, but I would like to `free` the buffer as a matter of code correctness.

This function also doesn't check the return result of `malloc`, which could lead to writing to `NULL` if the allocation fails.

My proposal is to add the following lines to this function:

	vfnt.glyphs = malloc(glyphsize);
+	if (vfnt.glyphs == NULL) {
+		perror("malloc");
+		return (1);
+	}

	if (fread(vfnt.glyphs, glyphsize, 1, f) != 1) {
		perror("glyphs");
+		free(vfnt.glyphs);
		return (1);
	}

	for (i = 0; i < VFNT_MAPS; i++)
		vfnt.map[i] = load_vt4mappingtable(vfnt.map_count[i], f);

	if (ioctl(STDIN_FILENO, PIO_VFONT, &vfnt) == -1) {
		perror("PIO_VFONT");
+		free(vfnt.glyphs);
		return (1);
	}
Comment 1 Ed Maste freebsd_committer 2016-10-07 14:34:44 UTC
Proposed change in https://reviews.freebsd.org/D8176
Comment 2 commit-hook freebsd_committer 2016-11-04 20:33:31 UTC
A commit references this bug:

Author: emaste
Date: Fri Nov  4 20:32:50 UTC 2016
New revision: 308312
URL: https://svnweb.freebsd.org/changeset/base/308312

Log:
  vidcontrol: improve error handling in vt(4) font loading

  PR:		209078
  Reported by:	ecturt@gmail.com
  Reviewed by:	Oliver Pinter
  Differential Revision:	https://reviews.freebsd.org/D8176

Changes:
  head/usr.sbin/vidcontrol/vidcontrol.c
Comment 3 commit-hook freebsd_committer 2017-02-10 14:59:23 UTC
A commit references this bug:

Author: emaste
Date: Fri Feb 10 14:58:24 UTC 2017
New revision: 313551
URL: https://svnweb.freebsd.org/changeset/base/313551

Log:
  MFC r308312: vidcontrol: improve error handling in vt(4) font loading

  PR:		209078

Changes:
_U  stable/11/
  stable/11/usr.sbin/vidcontrol/vidcontrol.c
Comment 4 commit-hook freebsd_committer 2017-02-10 15:03:28 UTC
A commit references this bug:

Author: emaste
Date: Fri Feb 10 15:02:56 UTC 2017
New revision: 313552
URL: https://svnweb.freebsd.org/changeset/base/313552

Log:
  MFC r308312: vidcontrol: improve error handling in vt(4) font loading

  PR:		209078

Changes:
_U  stable/10/
  stable/10/usr.sbin/vidcontrol/vidcontrol.c