Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

maybe_escape_markup: Make function memory-safe #526

Merged
merged 2 commits into from
May 8, 2024
Merged

maybe_escape_markup: Make function memory-safe #526

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6efb409
maybe_escape_markup: Make function memory-safe
orestisfl May 5, 2024
2e4812a
Revert to snprintf
orestisfl May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/i3status.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ void print_separator(const char *separator);
char *color(const char *colorstr);
char *endcolor() __attribute__((pure));
void reset_cursor(void);
void maybe_escape_markup(char *text, char **buffer);
char *maybe_escape_markup(char *text);

char *rtrim(const char *s);
char *ltrim(const char *s);
Expand Down
32 changes: 21 additions & 11 deletions src/output.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,39 +88,49 @@ void reset_cursor(void) {
* https://git.gnome.org/browse/glib/tree/glib/gmarkup.c?id=03db1f455b4265654e237d2ad55464b4113cba8a#n2142
*
*/
void maybe_escape_markup(char *text, char **buffer) {
char *maybe_escape_markup(char *text) {
if (markup_format == M_NONE) {
*buffer += sprintf(*buffer, "%s", text);
return;
return strdup(text);
}

size_t idx = 0;
size_t size = 32;
char *buffer = malloc(size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a slight preference for avoiding heap allocations.

Could we instead pass the size of the buffer and use snprintf to truncate? If you absolutely want to stick with allocations, I think we should just use asprintf instead of malloc/realloc/sprintf.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid allocations for performance reasons? I feel it will be a drop in the bucket with this amount of memory. I prefer to keep dynamic allocations to actually allocate the correct amount of memory.

Using asprintf will mean one allocation per character which seems excessive to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid allocations for performance reasons?

Yes. We rarely allocate in i3status (e.g. print_ipv6_addr dynamically allocates based on resp->ai_addrlen, though most of the remaining allocations could be optimized away, too).

Using asprintf will mean one allocation per character which seems excessive to me.

No, it will be one allocation per required escape, which seems reasonable. The advantage is that it gets rid of any manual allocation, so the code is easier to understand/verify overall.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about doubling the size of the essid buffer and using snprintf? This should make it memory safe and avoid any trimming except for specifically crafted essids with special symbols

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the change, tested with a STRING_SIZE of 5, 30, 60.

for (; *text != '\0'; text++) {
if (idx + 10 > size) {
size *= 2;
buffer = realloc(buffer, size);
}
switch (*text) {
case '&':
*buffer += sprintf(*buffer, "%s", "&");
idx += sprintf(&buffer[idx], "%s", "&");
break;
case '<':
*buffer += sprintf(*buffer, "%s", "&lt;");
idx += sprintf(&buffer[idx], "%s", "&lt;");
break;
case '>':
*buffer += sprintf(*buffer, "%s", "&gt;");
idx += sprintf(&buffer[idx], "%s", "&gt;");
break;
case '\'':
*buffer += sprintf(*buffer, "%s", "&apos;");
idx += sprintf(&buffer[idx], "%s", "&apos;");
break;
case '"':
*buffer += sprintf(*buffer, "%s", "&quot;");
idx += sprintf(&buffer[idx], "%s", "&quot;");
break;
default:
if ((0x1 <= *text && *text <= 0x8) ||
(0xb <= *text && *text <= 0xc) ||
(0xe <= *text && *text <= 0x1f)) {
*buffer += sprintf(*buffer, "&#x%x;", *text);
idx += sprintf(&buffer[idx], "&#x%x;", *text);
} else {
*(*buffer)++ = *text;
buffer[idx] = *text;
idx++;
}
break;
}
}
buffer[idx] = 0;
return buffer;
}

/*
Expand Down Expand Up @@ -154,4 +164,4 @@ char *trim(const char *s) {
char *f = ltrim(r);
free(r);
return f;
}
}
20 changes: 10 additions & 10 deletions src/print_wireless_info.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,9 +402,9 @@ static int get_wireless_info(const char *interface, wireless_info_t *info) {
close(s);
if (na.i_len >= sizeof(u.req)) {
/*
* Just use the first BSSID returned even if there are
* multiple APs sharing the same BSSID.
*/
* Just use the first BSSID returned even if there are
* multiple APs sharing the same BSSID.
*/
info->signal_level = u.req.info[0].isi_rssi / 2 +
u.req.info[0].isi_noise;
info->flags |= WIRELESS_INFO_FLAG_HAS_SIGNAL;
Expand Down Expand Up @@ -523,7 +523,7 @@ void print_wireless_info(wireless_info_ctx_t *ctx) {
/*
* Removing '%' and following characters from IPv6 since the interface identifier is redundant,
* as the output already includes the interface name.
*/
*/
if (ipv6_address != NULL) {
char *prct_ptr = strstr(ipv6_address, "%");
if (prct_ptr != NULL) {
Expand Down Expand Up @@ -569,7 +569,6 @@ void print_wireless_info(wireless_info_ctx_t *ctx) {
char string_quality[STRING_SIZE] = {'\0'};
char string_signal[STRING_SIZE] = {'\0'};
char string_noise[STRING_SIZE] = {'\0'};
char string_essid[STRING_SIZE] = {'\0'};
char string_frequency[STRING_SIZE] = {'\0'};
char string_ip[STRING_SIZE] = {'\0'};
char string_bitrate[STRING_SIZE] = {'\0'};
Expand Down Expand Up @@ -601,13 +600,13 @@ void print_wireless_info(wireless_info_ctx_t *ctx) {
snprintf(string_noise, STRING_SIZE, "?");
}

char *tmp = string_essid;
char *string_essid_tmp = NULL; /* Dynamic allocation of ESSID */
char *string_essid = "?";
#ifdef IW_ESSID_MAX_SIZE
if (info.flags & WIRELESS_INFO_FLAG_HAS_ESSID)
maybe_escape_markup(info.essid, &tmp);
else
if (info.flags & WIRELESS_INFO_FLAG_HAS_ESSID) {
string_essid = string_essid_tmp = maybe_escape_markup(info.essid);
}
#endif
snprintf(string_essid, STRING_SIZE, "?");

if (info.flags & WIRELESS_INFO_FLAG_HAS_FREQUENCY)
snprintf(string_frequency, STRING_SIZE, "%1.1f GHz", info.frequency / 1e9);
Expand All @@ -633,6 +632,7 @@ void print_wireless_info(wireless_info_ctx_t *ctx) {
char *formatted = format_placeholders(walk, &placeholders[0], num);
OUTPUT_FORMATTED;
free(formatted);
free(string_essid_tmp);

END_COLOR;
free(ipv4_address);
Expand Down
Loading