Skip to content

libcob: implement dynamic high-water mark screen_term_buff to prevent… #312

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

Open
arnab8335 wants to merge 2 commits into
base: gitside-gnucobol-3.x
Choose a base branch
from
+119 −29
Open

libcob: implement dynamic high-water mark screen_term_buff to prevent… #312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6427fd3
libcob: implement dynamic high-water mark screen_term_buff to prevent…
arnab8335 Aug 30, 2026
8ff350f
libcob: resolve formatting and compiler warning checks
arnab8335 Aug 30, 2026
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
3 changes: 3 additions & 0 deletions libcob/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,9 @@ typedef __mpz_struct mpz_t[1];
TODO: add compiler configuration for limiting this */
#define COB_MAX_SUBSCRIPTS 16

/* Maximum Buffer Warning Size */
#define COB_TERM_BUFF_WARN_SIZE 32768

/* Memory size for sorting */
#define COB_SORT_MEMORY 128 * 1024 * 1024
#define COB_SORT_CHUNK 256 * 1024
Expand Down
105 changes: 76 additions & 29 deletions libcob/screenio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,9 @@ static int pending_accept;
static int got_sys_char;
static int save_cursor_x = 0;
static int save_cursor_y = 0;
static WINDOW *mywin;
static WINDOW *mywin;
static char *screen_term_buff = NULL;
static size_t screen_buffer_size = 0;

#ifdef WITH_PANELS
#define MAX_PANELS 20
Expand Down Expand Up @@ -3530,6 +3532,41 @@ field_display (cob_field *f, cob_flags_t fattr, const int line, const int column
refresh_mywin (mywin);
}

static char *
ensure_buffer (size_t requested_size)
{
if (screen_buffer_size == 0) {
screen_buffer_size = COB_MINI_BUFF;
}
Comment on lines +3538 to +3540

@GitMensch GitMensch Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (screen_buffer_size == 0) {
screen_buffer_size = COB_MINI_BUFF;
}
if (screen_buffer_size == 0) {
/* ensure minimal buffer at first use */
if (requested_size > COB_MINI_BUFF) {
screen_buffer_size = requested_size;
} else {
screen_buffer_size = COB_MINI_BUFF;
}
screen_term_buff = cob_fast_malloc (screen_buffer_size);
return screen_term_buff;
}

Note: I've moved the allocation from down there here, and don't do an extra-check of the initial cob_fast_malloc... but we may should do that, given that the first allocation can be huge as well


if (requested_size > screen_buffer_size) {
char *tmp;

if (requested_size > COB_TERM_BUFF_WARN_SIZE) {
cob_runtime_warning (_("ACCEPT/DISPLAY of unusually large field (%lu bytes)"),
(unsigned long)requested_size);
Comment on lines +3546 to +3547

@GitMensch GitMensch Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Better move the size casting outside, for example by making it unsigned long requested_size.

I'm not sure if it is better to have that warning only once per increase - as done here - or at the start of this function (both options have benefits and issues); I tend to the second (otherwise a bit reorderning is needed to do that before the == 0 case above

}

/* Expanding the Buffer */
tmp = realloc (screen_term_buff, requested_size);
if (tmp == NULL) {
cob_runtime_error (_("could not allocate %lu bytes of memory"),
(unsigned long)requested_size);
return screen_term_buff;
}
screen_term_buff = tmp;
screen_buffer_size = requested_size;
} else if (screen_term_buff == NULL) {
screen_term_buff = realloc (screen_term_buff, screen_buffer_size);

@GitMensch GitMensch Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

as we already have an explicit path for "did not work" and don't need to retain the old content: please use cob_fast_malloc + cob_free here instead of plain realloc...

And yes, you're right that cob_malloc would potentially abort if too big (which is unlikely but still possible) - there is a pending TODO to work on this by providing a non-aborting version - feel free to add a special cob_fast_malloc version which never aborts [using that will mean the caller need to check, like done here)

if (screen_term_buff == NULL) {
cob_runtime_error (_("could not allocate %lu bytes of memory"),
(unsigned long)screen_buffer_size);
}
Comment on lines +3559 to +3564

@GitMensch GitMensch Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
} else if (screen_term_buff == NULL) {
screen_term_buff = realloc (screen_term_buff, screen_buffer_size);
if (screen_term_buff == NULL) {
cob_runtime_error (_("could not allocate %lu bytes of memory"),
(unsigned long)screen_buffer_size);
}

new code checks that up-front

}

return screen_term_buff;
}

static void
field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolumn,
cob_field *fgc, cob_field *bgc, cob_field *fscroll, cob_field *ftimeout,
Expand All @@ -3538,6 +3575,7 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
{
unsigned char *p;
unsigned char *p2;
unsigned char *term_buff;

@GitMensch GitMensch Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if we use the static buffer only here then we should use the static var in this function as well, not a local copy

size_t count;
int keyp;
int fret = 0;
Expand Down Expand Up @@ -3612,15 +3650,20 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
} else {
size_accept = f->size;
}
term_buff = (unsigned char *)ensure_buffer (size_accept);

@GitMensch GitMensch Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
term_buff = (unsigned char *)ensure_buffer (size_accept);
}
if (size_accept) { /* non-zero-sized field */
term_buff = (unsigned char *)ensure_buffer (size_accept);

this effectively handles a variable-length field of size zero like ACCEPT OMITTED - and we don't have to care for zero-size buffer later

@GitMensch GitMensch Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

by using the static var, this function can be changed to return 0 on success, 1 on failure and we can check the sucess this way

if (screen_buffer_size < size_accept) {
/* allocation failure */
return;
Comment on lines +3655 to +3656

@GitMensch GitMensch Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should raise an exception here (add to exceptions.def, then use the define here; possibly a new, non-fatal one EC-SCREEN-IMP-STORAGE?) and only return if the size is zero - otherwise set the size_accept to the actual size.

}

p = COB_TERM_BUFF;
temp_field.data = COB_TERM_BUFF;
p = term_buff;
temp_field.data = term_buff;
temp_field.attr = &const_alpha_attr;
temp_field.size = size_accept;
if (fattr & COB_SCREEN_UPDATE) {
cob_move (f, &temp_field); /* updates COB_TERM_BUFF */
cob_move (f, &temp_field); /* updates term_buff */
} else {
memset (COB_TERM_BUFF, ' ', size_accept);
memset (term_buff, ' ', size_accept);
}

raise_ec_on_truncation (size_accept);
Expand Down Expand Up @@ -3657,7 +3700,7 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
cursor_off = cob_get_int (cursor);
if (cursor_off >= 1) {
/* max: last_position with data */
int last_data = p_set - COB_TERM_BUFF + 1;
int last_data = p_set - term_buff + 1;
if (last_data < cursor_off) {
cursor_off = last_data;
}
Expand Down Expand Up @@ -3694,7 +3737,7 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
accept_cursor_x = scolumn + size_accept;

right_pos = scolumn + size_accept - 1;
p = COB_TERM_BUFF;
p = term_buff;
} else {
right_pos = 0;
p = NULL;
Expand All @@ -3721,7 +3764,7 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
}
for (count = right_pos; (int)count > scolumn - 1; count--) {
/* Get character */
p2 = COB_TERM_BUFF + count - scolumn;
p2 = term_buff + count - scolumn;
move_char = *p2;
/* Field prompts. */
if (COB_FIELD_IS_NUMERIC (f)) {
Expand Down Expand Up @@ -3902,10 +3945,10 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
/* Shift remainder left with cursor. */
for (count = ccolumn; count < right_pos + 1; count++) {
/* Get character. */
p2 = COB_TERM_BUFF + count - scolumn ;
p2 = term_buff + count - scolumn ;
move_char = *p2;
/* Move the character left. */
p2 = COB_TERM_BUFF + count - scolumn - 1;
p2 = term_buff + count - scolumn - 1;
*p2 = move_char;
/* Update screen with moved character. */
cob_move_cursor (cline, count - 1);
Expand All @@ -3918,7 +3961,7 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
}
}
/* Put space as the right most character. */
p2 = COB_TERM_BUFF + size_accept - 1;
p2 = term_buff + size_accept - 1;
if (fattr & COB_SCREEN_NO_ECHO) {
*p2 = COB_CH_SP;
} else if (COB_FIELD_IS_NUMERIC (f)) {
Expand All @@ -3942,7 +3985,7 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
/* Find non-blank character left to right. */
for (count = scolumn; count <= right_pos; count++) {
/* Get character. */
p2 = COB_TERM_BUFF + count - scolumn;
p2 = term_buff + count - scolumn;
move_char = *p2;
/* Stop at beginning non-blank character. */
if (move_char != ' ') {
Expand All @@ -3958,11 +4001,11 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
/* Cursor to start of characters. */
ccolumn = count;
cob_move_cursor (cline, ccolumn);
p = COB_TERM_BUFF + ccolumn - scolumn;
p = term_buff + ccolumn - scolumn;
} else {
/* Cursor to start of field. */
cob_move_cursor (sline, scolumn);
p = COB_TERM_BUFF;
p = term_buff;
}
/* Reset */
at_eof = 0;
Expand All @@ -3974,7 +4017,7 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
/* Find non-blank character right to left. */
for (count = right_pos; (int) count >= scolumn; count--) {
/* Get character. */
p2 = COB_TERM_BUFF + count - scolumn;
p2 = term_buff + count - scolumn;
move_char = *p2;
/* Stop at ending non-blank character. */
if (move_char != ' ') {
Expand All @@ -3995,11 +4038,11 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
/* Cursor after end character. */
ccolumn = count;
cob_move_cursor (cline, ccolumn);
p = COB_TERM_BUFF + ccolumn - scolumn;
p = term_buff + ccolumn - scolumn;
} else {
/* Cursor to end of size of field */
cob_move_cursor (sline, right_pos);
p = COB_TERM_BUFF + size_accept - 1;
p = term_buff + size_accept - 1;
}
/* Reset */
at_eof = 0;
Expand All @@ -4012,7 +4055,7 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
if ((int) ccolumn > scolumn) {
ccolumn--;
cob_move_cursor (cline, ccolumn);
p = COB_TERM_BUFF + ccolumn - scolumn;
p = term_buff + ccolumn - scolumn;
continue;
}
/* End of field, auto-skip, return left-arrow. */
Expand All @@ -4029,7 +4072,7 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
if (ccolumn < right_pos) {
ccolumn++;
cob_move_cursor (cline, ccolumn);
p = COB_TERM_BUFF + ccolumn - scolumn;
p = term_buff + ccolumn - scolumn;
continue;
}
/* End of field, auto-skip, return right-arrow. */
Expand All @@ -4048,10 +4091,10 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
/* Delete character, move remainder left. */
for (count = ccolumn; count < right_pos; count++) {
/* Get character one position to right. */
p2 = COB_TERM_BUFF + count - scolumn + 1;
p2 = term_buff + count - scolumn + 1;
move_char = *p2;
/* Move the character left. */
p2 = COB_TERM_BUFF + count - scolumn;
p2 = term_buff + count - scolumn;
*p2 = move_char;
/* Update screen with moved character. */
cob_move_cursor (cline, count);
Expand All @@ -4064,7 +4107,7 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
}
}
/* Put space as the right most character. */
p2 = COB_TERM_BUFF + size_accept - 1;
p2 = term_buff + size_accept - 1;
if (fattr & COB_SCREEN_NO_ECHO) {
*p2 = COB_CH_SP;
} else if (COB_FIELD_IS_NUMERIC (f)) {
Expand All @@ -4079,7 +4122,7 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
/* Alt-Delete key, erase cursor to end of field. */
for (count = ccolumn; count <= right_pos; count++) {
/* Character position. */
p2 = COB_TERM_BUFF + count - scolumn;
p2 = term_buff + count - scolumn;
/* Blank character. */
if (fattr & COB_FIELD_IS_NUMERIC (f)) {
move_char = '0';
Expand Down Expand Up @@ -4119,7 +4162,7 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
&& mcolumn <= (int)right_pos) {
ccolumn = mcolumn;
cob_move_cursor (cline, ccolumn);
p = COB_TERM_BUFF + ccolumn - scolumn;
p = term_buff + ccolumn - scolumn;
continue;
}
}
Expand Down Expand Up @@ -4169,7 +4212,7 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
if (cob_field_is_numeric_or_numeric_edited (f)) {
p2 = (unsigned char *)" ";
} else {
p2 = COB_TERM_BUFF + right_pos - scolumn;
p2 = term_buff + right_pos - scolumn;
}
if (*p2 != ' ') {
cob_beep ();
Expand All @@ -4178,10 +4221,10 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
/* Move remainder to the right. */
for (count = right_pos; count > ccolumn; count--) {
/* Get character */
p2 = COB_TERM_BUFF + count - scolumn - 1;
p2 = term_buff + count - scolumn - 1;
move_char = *p2;
/* Move character one right. */
p2 = COB_TERM_BUFF + count - scolumn;
p2 = term_buff + count - scolumn;
*p2 = move_char;
/* Update screen with moved character. */
if ((int) count > scolumn) {
Expand Down Expand Up @@ -4254,7 +4297,7 @@ field_accept (cob_field *f, cob_flags_t fattr, const int sline, const int scolum
cob_move (&temp_field, f);
cob_move_cursor (sline, right_pos + 1);
#if 0 /* possible cleanup to not "leak" input data */
memset (COB_TERM_BUFF, ' ', size_accept);
memset (term_buff, ' ', size_accept);
#endif
}
refresh_mywin (mywin);
Expand Down Expand Up @@ -4713,6 +4756,11 @@ cob_exit_screen (void)
cob_free (cob_base_inp);
cob_base_inp = NULL;
}
if (screen_term_buff) {
free (screen_term_buff);

@GitMensch GitMensch Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
free (screen_term_buff);
cob_free (screen_term_buff);

screen_term_buff = NULL;
screen_buffer_size = 0;
}
}
COB_ACCEPT_STATUS = 0;
cobglobptr = NULL;
Expand Down Expand Up @@ -5681,7 +5729,6 @@ sys_window_delete (PPARM_FLD parm)
return 0;
}


/* LIST_WINDOW - list all of the windows */
static void
sys_window_list (PLIST_FLD list, size_t amount)
Expand Down
40 changes: 40 additions & 0 deletions tests/testsuite.src/run_manual_screen.at
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4159,3 +4159,43 @@ AT_CHECK([$COMPILE prog.cob], [0], [], [])
MANUAL_CHECK([$COBCRUN_DIRECT ./prog], [0], [], [])

AT_CLEANUP


AT_SETUP([ACCEPT field larger than internal I/O buffer])
AT_KEYWORDS([screen accept])

AT_SKIP_IF([test "$COB_HAS_CURSES" != "yes"])

# regression test: the internal screen I/O buffer used to be a fixed
# 8192 bytes (COB_MEDIUM_BUFF); ACCEPTing into a field larger than that
# overflowed it. The buffer now grows on demand, so this must complete
# (and not crash) for a field well past that old limit.
AT_DATA([prog.cob], [
IDENTIFICATION DIVISION.
PROGRAM-ID. prog.
DATA DIVISION.
WORKING-STORAGE SECTION.
01 big-field PIC X(9000).

SCREEN SECTION.
01 scr.
03 LINE 1 VALUE
'Type a few characters, then press ENTER.'.
03 LINE 3 COL 1 PIC X(9000) USING big-field.
Comment on lines +4181 to +4184

@GitMensch GitMensch Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the test is good; for full test coverage we'd need an additional accept > warning threshold as well (then checking its diagnostic as well)


PROCEDURE DIVISION.
DISPLAY scr
ACCEPT scr

IF COB-CRT-STATUS = 0
GOBACK RETURNING 0
ELSE
GOBACK RETURNING 1
END-IF
.
])

AT_CHECK([$COMPILE prog.cob], [0], [], [])
MANUAL_CHECK([$COBCRUN_DIRECT ./prog], [0], [], [])

AT_CLEANUP
Loading