libcob: implement dynamic high-water mark screen_term_buff to prevent… - #312
libcob: implement dynamic high-water mark screen_term_buff to prevent…#312arnab8335 wants to merge 2 commits into
Conversation
GitMensch
left a comment
GitMensch
left a comment
There was a problem hiding this comment.
The general approach is good, thanks for the PR!
TODOs:
- COB_TERM_BUFF (cobolocal.h) should be removed in this PR, as well as the struct element it points to
- exception to be added
- libcob/Changelog needs an entry
Please note https://sourceforge.net/p/gnucobol/wiki/Copyright%20reassignment/ as we're reaching the threshold of legally significance - especially if you can see yourself doing more contributions.
| /* allocation failure */ | ||
| return; |
There was a problem hiding this comment.
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.
| } else { | ||
| size_accept = f->size; | ||
| } | ||
| term_buff = (unsigned char *)ensure_buffer (size_accept); |
There was a problem hiding this comment.
| 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
| cob_runtime_warning (_("ACCEPT/DISPLAY of unusually large field (%lu bytes)"), | ||
| (unsigned long)requested_size); |
There was a problem hiding this comment.
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
| if (screen_buffer_size == 0) { | ||
| screen_buffer_size = COB_MINI_BUFF; | ||
| } |
There was a problem hiding this comment.
| 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
| } 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); | ||
| } |
There was a problem hiding this comment.
| } 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
| { | ||
| unsigned char *p; | ||
| unsigned char *p2; | ||
| unsigned char *term_buff; |
There was a problem hiding this comment.
if we use the static buffer only here then we should use the static var in this function as well, not a local copy
| } else { | ||
| size_accept = f->size; | ||
| } | ||
| term_buff = (unsigned char *)ensure_buffer (size_accept); |
There was a problem hiding this comment.
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
| 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); |
There was a problem hiding this comment.
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)
| cob_base_inp = NULL; | ||
| } | ||
| if (screen_term_buff) { | ||
| free (screen_term_buff); |
There was a problem hiding this comment.
| free (screen_term_buff); | |
| cob_free (screen_term_buff); |
| 01 scr. | ||
| 03 LINE 1 VALUE | ||
| 'Type a few characters, then press ENTER.'. | ||
| 03 LINE 3 COL 1 PIC X(9000) USING big-field. |
There was a problem hiding this comment.
the test is good; for full test coverage we'd need an additional accept > warning threshold as well (then checking its diagnostic as well)
|
Hello, @GitMensch |
|
Hi @arnab8335. Thanks for your honesty - it is good to know where the current guidelines are. The symbol-renaming part is insignificant - so "most" LOC aren't. For the other parts: if you rewrite them per review comments, I'm quite sure that those more significant memory blocks are also rewritten, so that's fine with me and I don't mind if you work from the current PR's state or from scratch. We'd still need the copyright papers (or at least disclaimers) which can take a while so you may consider starting that procedure, especially if you can see yourself to stay (we have a lot of easy to complex open tasks. For regular contributors we do not only offer (hopefully clear and informative) reviews but also mentoring, if wanted. |
|
Hi @GitMensch |
2 participants
Fix: heap overflow in field_accept() for ACCEPT fields larger than the fixed screen I/O buffer
Implemented the dynamic buffer approach you suggested, scoped entirely to screenio.c:
Added a file-local static char *screen_term_buff / static size_t screen_buffer_size, separate from the shared cobglobptr->cob_term_buff used by termio.c — so this only affects extended screen I/O, per your note.
ensure_buffer(size_t requested_size): starts at a 256-byte baseline (COB_MINI_BUFF), grows via raw realloc (not cob_malloc/cob_realloc, since those hard-abort on OOM and the requested size comes straight from the COBOL field — in theory up to ~2GB). Never shrinks; grows to exactly the requested size (high-water mark), so repeated ACCEPTs at similar sizes don't re-realloc.
Warns via cob_runtime_warning() (non-fatal) when a grow exceeds COB_TERM_BUFF_WARN_SIZE (32KB, now a #define), and reports via cob_runtime_error() on actual allocation failure — the field is still accepted at full size in the warning case; only real OOM causes field_accept() to bail out of the ACCEPT (checked via screen_buffer_size < size_accept after calling ensure_buffer, so a failed grow can't fall through into writing past the old, smaller buffer).
Wired into field_accept(): replaced every use of the COB_TERM_BUFF macro within that function with a local term_buff pointer from ensure_buffer(). COB_TERM_BUFF itself is untouched for termio.c.
Freed in cob_exit_screen(), alongside the existing cob_base_inp cleanup, and reset to NULL/0 so re-entering screen mode later in the same process starts clean.
Testing:
Added a regression test to tests/testsuite.src/run_manual_screen.at (ACCEPT field larger than internal I/O buffer) using a PIC X(9000) screen field, past the old 8192-byte ceiling.
Verified with AddressSanitizer: rebuilt screenio.c under ASan, ran the 9000-byte ACCEPT through cobcrun — no overflow/crash, versus the old fixed-buffer code which would have overflowed on this input.