From: igor@9lab.org
To: 9front@9front.org
Cc: igor@9lab.org
Subject: [9front] acme: fix leaking memory allocated by getenv("font")
Date: Sat, 06 Nov 2021 01:05:50 +0100 [thread overview]
Message-ID: <CEE2988A15F7E9CF961E39FBE74B7488@9lab.org> (raw)
[-- Attachment #1: Type: text/plain, Size: 3470 bytes --]
The attached patch fixes a memory leak that occurs on a code
path where the acme font is retrieved via `getenv("font")`.
It is a small leak. Its presence is still annoying as it
pollutes leak(1) output for acme.
I am posting the patch here first in case there is review feedback.
If no one objects I will push it over the weekend.
Cheers,
Igor
<snip>
From: Igor Böhm <igor@9lab.org>
Date: Fri, 05 Nov 2021 23:51:55 +0000
Subject: [PATCH] acme: fix leaking memory allocated by getenv("font")
If the font chosen for acme is retrieved via `getenv("font")` its
memory is leaked:
<snip>
if(fontnames[0] == nil)
fontnames[0] = getenv("font");
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> getenv(…) mallocs memory
if(fontnames[0] == nil)
fontnames[0] = "/lib/font/bit/vga/unicode.font";
if(access(fontnames[0], 0) < 0){
fprint(2, "acme: can't access %s: %r\n", fontnames[0]);
exits("font open");
}
if(fontnames[1] == nil)
fontnames[1] = fontnames[0];
fontnames[0] = estrdup(fontnames[0]);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> if the `getenv("font")` path was taken above, this assignment
> will leak its memory.
</snap>
The following leak/acid session demonstrates the issue:
<snip>
cpu% leak -s 212252
src(0x002000cb); // 1
cpu% acid 212252
/proc/212252/text:amd64 plan 9 executable
/sys/lib/acid/port
/sys/lib/acid/amd64
acid: src(0x002000cb)
/sys/src/cmd/acme/acme.c:107
102 fprint(2, "usage: acme [-aib] [-c ncol] [-f font] [-F fixedfont] [-l loadfile | file...]\n");
103 exits("usage");
104 }ARGEND
105
106 if(fontnames[0] == nil)
>107 fontnames[0] = getenv("font");
108 if(fontnames[0] == nil)
109 fontnames[0] = "/lib/font/bit/vga/unicode.font";
110 if(access(fontnames[0], 0) < 0){
111 fprint(2, "acme: can't access %s: %r\n", fontnames[0]);
112 exits("font open");
acid:
</snap>
The fix tries to first check if a font has been set via
command line options in which case the font string is
malloced via estrdup(…).
If no font has been selected on the command line getenv("font")
is used. If no getenv("font") var is found we malloc a default
font via estrdup(…).
<snip>
if(fontnames[0] != nil)
fontnames[0] = estrdup(fontnames[0]);
else
if((fontnames[0] = getenv("font")) == nil)
fontnames[0] = estrdup("/lib/font/bit/vga/unicode.font");
if(access(fontnames[0], 0) < 0){
fprint(2, "acme: can't access %s: %r\n", fontnames[0]);
exits("font open");
}
if(fontnames[1] == nil)
fontnames[1] = fontnames[0];
fontnames[1] = estrdup(fontnames[1]);
</snap>
This resolves the memory leak and indeed leak(1) will not report
any leaks ☺
---
diff 775608db7c004aca265474adb3e081f676b3b12a cb2f16fd1b18ff6bf12b594d129b8ff54423cb58
--- a/sys/src/cmd/acme/acme.c Fri Nov 5 00:11:56 2021
+++ b/sys/src/cmd/acme/acme.c Sat Nov 6 00:51:55 2021
@@ -103,17 +103,17 @@
exits("usage");
}ARGEND
- if(fontnames[0] == nil)
- fontnames[0] = getenv("font");
- if(fontnames[0] == nil)
- fontnames[0] = "/lib/font/bit/vga/unicode.font";
+ if(fontnames[0] != nil)
+ fontnames[0] = estrdup(fontnames[0]);
+ else
+ if((fontnames[0] = getenv("font")) == nil)
+ fontnames[0] = estrdup("/lib/font/bit/vga/unicode.font");
if(access(fontnames[0], 0) < 0){
fprint(2, "acme: can't access %s: %r\n", fontnames[0]);
exits("font open");
}
if(fontnames[1] == nil)
fontnames[1] = fontnames[0];
- fontnames[0] = estrdup(fontnames[0]);
fontnames[1] = estrdup(fontnames[1]);
quotefmtinstall();
</snap>
[-- Attachment #2: acme.getenv.font.patch --]
[-- Type: text/plain, Size: 3105 bytes --]
From: Igor Böhm <igor@9lab.org>
Date: Fri, 05 Nov 2021 23:51:55 +0000
Subject: [PATCH] acme: fix leaking memory allocated by getenv("font")
If the font chosen for acme is retrieved via `getenv("font")` its
memory is leaked:
<snip>
if(fontnames[0] == nil)
fontnames[0] = getenv("font");
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> getenv(…) mallocs memory
if(fontnames[0] == nil)
fontnames[0] = "/lib/font/bit/vga/unicode.font";
if(access(fontnames[0], 0) < 0){
fprint(2, "acme: can't access %s: %r\n", fontnames[0]);
exits("font open");
}
if(fontnames[1] == nil)
fontnames[1] = fontnames[0];
fontnames[0] = estrdup(fontnames[0]);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> if the `getenv("font")` path was taken above, this assignment
> will leak its memory.
</snap>
The following leak/acid session demonstrates the issue:
<snip>
cpu% leak -s 212252
src(0x002000cb); // 1
cpu% acid 212252
/proc/212252/text:amd64 plan 9 executable
/sys/lib/acid/port
/sys/lib/acid/amd64
acid: src(0x002000cb)
/sys/src/cmd/acme/acme.c:107
102 fprint(2, "usage: acme [-aib] [-c ncol] [-f font] [-F fixedfont] [-l loadfile | file...]\n");
103 exits("usage");
104 }ARGEND
105
106 if(fontnames[0] == nil)
>107 fontnames[0] = getenv("font");
108 if(fontnames[0] == nil)
109 fontnames[0] = "/lib/font/bit/vga/unicode.font";
110 if(access(fontnames[0], 0) < 0){
111 fprint(2, "acme: can't access %s: %r\n", fontnames[0]);
112 exits("font open");
acid:
</snap>
The fix tries to first check if a font has been set via
command line options in which case the font string is
malloced via estrdup(…).
If no font has been selected on the command line getenv("font")
is used. If no getenv("font") var is found we malloc a default
font via estrdup(…).
<snip>
if(fontnames[0] != nil)
fontnames[0] = estrdup(fontnames[0]);
else
if((fontnames[0] = getenv("font")) == nil)
fontnames[0] = estrdup("/lib/font/bit/vga/unicode.font");
if(access(fontnames[0], 0) < 0){
fprint(2, "acme: can't access %s: %r\n", fontnames[0]);
exits("font open");
}
if(fontnames[1] == nil)
fontnames[1] = fontnames[0];
fontnames[1] = estrdup(fontnames[1]);
</snap>
This resolves the memory leak and indeed leak(1) will not report
any leaks ☺
---
diff 775608db7c004aca265474adb3e081f676b3b12a cb2f16fd1b18ff6bf12b594d129b8ff54423cb58
--- a/sys/src/cmd/acme/acme.c Fri Nov 5 00:11:56 2021
+++ b/sys/src/cmd/acme/acme.c Sat Nov 6 00:51:55 2021
@@ -103,17 +103,17 @@
exits("usage");
}ARGEND
- if(fontnames[0] == nil)
- fontnames[0] = getenv("font");
- if(fontnames[0] == nil)
- fontnames[0] = "/lib/font/bit/vga/unicode.font";
+ if(fontnames[0] != nil)
+ fontnames[0] = estrdup(fontnames[0]);
+ else
+ if((fontnames[0] = getenv("font")) == nil)
+ fontnames[0] = estrdup("/lib/font/bit/vga/unicode.font");
if(access(fontnames[0], 0) < 0){
fprint(2, "acme: can't access %s: %r\n", fontnames[0]);
exits("font open");
}
if(fontnames[1] == nil)
fontnames[1] = fontnames[0];
- fontnames[0] = estrdup(fontnames[0]);
fontnames[1] = estrdup(fontnames[1]);
quotefmtinstall();
reply other threads:[~2021-11-06 0:26 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CEE2988A15F7E9CF961E39FBE74B7488@9lab.org \
--to=igor@9lab.org \
--cc=9front@9front.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).