Fix BLANK WHEN ZERO on signed NUMERIC-EDITED #201

ddeclerck · 2024-12-09T10:43:24Z

On behalf of Chuck.

GitMensch

Thanks for your work on this again.

Please recheck the leading sign issue by adjusting the test case.

GitMensch · 2024-12-09T12:26:38Z

libcob/move.c

+		if (COB_FIELD_HAVE_SIGN (f1) && COB_FIELD_SIGN_SEPARATE (f1)) {
+			for (; (src < src_end) ; src++) {
+				if (*src != '0') break;
+			}
+			if (src >= src_end) {
+				memset (dst, (int)' ', (size_t)f2->size);
+				return;
+			}
+		} else {
+			for (; (src <= src_end) ; src++) {
+				if (*src != '0') break;
+			}
+			if (src > src_end) {
+				memset (dst, (int)' ', (size_t)f2->size);
+				return;
+			}


Please change it to use something like const unsigned char *check_end = !COB_FIELD_SIGN_SEPARATE (f1) ? src_end : src_end - 1; instead to reduce the duplication.

Also: What about separate and leading sign?
And... where are the overpunched positions checked? As we just had that in the other PR: What about invalid data - should we check with COB_D2I() = 0 instead?

Note: I think there's no use in explicit casting a char to int, is it? The field->size is size_t already, so that should be dropped in any case.

Also: What about separate and leading sign? And... where are the overpunched positions checked?

There is an invariant here that f1 either has no sign or has a trailing separate sign (as checked by the block between #ifndef NDEBUG block at the top of the function, and also mentionned by Chuck in his message).

As we just had that in the other PR: What about invalid data - should we check with COB_D2I() = 0 instead?

We could add that, though the other PR is about moving from an ALPHANUMERIC source, while optimized_move_display_to_edited only moves from a NUMERIC-DISPLAY source (either a direct one, or a temporary one).

Note: I think there's no use in explicit casting a char to int, is it?

Absolutely, those character literals should already be of type int anyways.

The field->size is size_t already, so that should be dropped in any case.

Done.

There is an invariant here that f1 either has no sign or has a trailing separate sign (as checked by the block between #ifndef NDEBUG block at the top of the function, and also mentioned by Chuck in his message).

Can you expand that to also the leading separate case (should be simple, and we now even have a testcase ready :-) and to the overpunched sign (a bit more complex)?
Of course we can postpone that to later (either both or one of those)...

As we just had that in the other PR: What about invalid data - should we check with COB_D2I() = 0 instead?

We could add that, though the other PR is about moving from an ALPHANUMERIC source, while optimized_move_display_to_edited only moves from a NUMERIC-DISPLAY source (either a direct one, or a temporary one).

Just checked with MF: After MOVE SPACES TO L(1:3) before the W-X move, by default we get the abort for non-numeric data in numeric displays, when I disable that at run-time, then " 00000" is blanked to space and " 1234" results in " 12,34" while an additional MOVE "A" TO L (8:1) results in

===> 0,01<=== ===> 12,31<===

I guess this is the same for GCOS, in this case we may should at this at the end of that test (and use COB_D2I(c) == 0).

Can you expand that to also the leading separate case (should be simple, and we now even have a testcase ready :-) and to the overpunched sign (a bit more complex)? Of course we can postpone that to later (either both or one of those)...

Done for this part.

Just checked with MF: After MOVE SPACES TO L(1:3) before the W-X move, by default we get the abort for non-numeric data in numeric displays, when I disable that at run-time, then " 00000" is blanked to space and " 1234" results in " 12,34" while an additional MOVE "A" TO L (8:1) results in

===> 0,01<=== ===> 12,31<===

Added this to the test case, though the result differs, i.e.:

MOVE 0 TO L. MOVE SPACES TO L(1:3). MOVE L TO W-X. DISPLAY ' ===>' W-X '<==='. MOVE "A" TO L(8:1). DISPLAY ' ===>' W-X '<==='. MOVE 1234 TO L. MOVE SPACES TO L(1:3). MOVE L TO W-X. DISPLAY ' ===>' W-X '<==='. MOVE "A" TO L(8:1). DISPLAY ' ===>' W-X '<==='.

Yields:

===> <=== ===> <=== ===> 012,34<=== ===> 012,34<===

Though this is not really related to the "BLANK WHEN ZERO not working anymore" issue.

tests/testsuite.src/run_fundamental.at

GitMensch

I do question if GCOS would output 012,34...

please see some notes and add a Changelog (and yes, that part is a separate one from Chucks fixing something else - still fine for a single commit as that fixes two errors in the same function that are verified by a single test.

... it seems reasonable to check the CI generated coverage as well, to ensure that at least all parts of this function that is now fixed is actually tested (or at least: executed).

libcob/move.c

tests/testsuite.src/run_fundamental.at

libcob/move.c

tests/testsuite.src/run_fundamental.at

ddeclerck · 2024-12-12T17:18:39Z

I think I considered all requested changes.

libcob/ChangeLog

libcob/move.c

tests/testsuite.src/run_fundamental.at

GitMensch · 2024-12-13T11:19:38Z

Sounds I do wait for the next push, right?

ddeclerck · 2024-12-13T11:24:22Z

Yes.

GitMensch

seems good to me, apart from the Changelog entry we need to fix run_misc.at - the test "Figurative constants to numeric field" (which should get an additional keyword "MOVE") doers not only test what it says, but additional the possible pretty-printing - which it shouldn't.

please change is " NUM9 "." to is " NUM9 (1:) "."

to fix that test

and yes, we have one additional issue which was seen by this: the pretty-printing of USAGE DISPLAY is different in at least those compilers:
acu: no pretty-printing on those at all
mf: all signed fields are printed as if they'd have a "SIGN SEPARATE TRAILING"
gc: full pretty-printing (including decimals, normalization, ...)

but that's something to only create a FR on "provide different pretty-display for USAGE DISPLAY" (and I don't consider that a high priority)

libcob/ChangeLog

ddeclerck · 2024-12-17T14:07:37Z

Here we go.

I also sneaked into this the small testsuite adjustment mentionned in your last comment in #203.

GitMensch

Good as-is.

ddeclerck · 2024-12-17T15:48:27Z

Merged in SVN @ 5403.

GitMensch · 2024-12-20T12:20:58Z

Happens since this commit - so far only in NC105A

==80021==ERROR: AddressSanitizer: global-buffer-overflow on address 0x5558a5310e65 at pc 0x7fb85f81f06f bp 0x7ffc67696d20 sp 0x7ffc67696d10
READ of size 1 at 0x5558a5310e65 thread T0
    #0 0x7fb85f81f06e in optimized_move_display_to_edited /home/appveyor/projects/gnucobol-3-x/libcob/move.c:1117
    #1 0x7fb85f826e2d in cob_move /home/appveyor/projects/gnucobol-3-x/libcob/move.c:1699
    #2 0x5558a4d0683b in NC105A_ (/home/appveyor/projects/gnucobol-3-x/tests/cobol85/NC/NC105A+0x3cb83b)
    #3 0x5558a4ca87e0 in NC105A (/home/appveyor/projects/gnucobol-3-x/tests/cobol85/NC/NC105A+0x36d7e0)
    #4 0x5558a4ca8797 in main (/home/appveyor/projects/gnucobol-3-x/tests/cobol85/NC/NC105A+0x36d797)
    #5 0x7fb85ec47082 in __libc_start_main ../csu/libc-start.c:308
    #6 0x5558a4ca86ad in _start (/home/appveyor/projects/gnucobol-3-x/tests/cobol85/NC/NC105A+0x36d6ad)

0x5558a5310e65 is located 0 bytes to the right of global variable 'b_111' defined in '/tmp/cob79759_0.c.l.h:101:17' (0x5558a5310e60) of size 5
0x5558a5310e65 is located 59 bytes to the left of global variable 'b_10' defined in '/tmp/cob79759_0.c.l.h:58:17' (0x5558a5310ea0) of size 4
SUMMARY: AddressSanitizer: global-buffer-overflow /home/appveyor/projects/gnucobol-3-x/libcob/move.c:1117 in optimized_move_display_to_edited
Shadow bytes around the buggy address:
  0x0aab94a5a170: 00 00 00 00 00 00 00 00 00 00 00 f9 f9 f9 f9 f9
  0x0aab94a5a180: 05 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 00 00
  0x0aab94a5a190: 00 00 00 00 00 00 00 f9 f9 f9 f9 f9 00 00 00 00
  0x0aab94a5a1a0: 00 00 00 00 00 00 00 00 00 00 00 f9 f9 f9 f9 f9
  0x0aab94a5a1b0: 00 00 00 00 00 00 f9 f9 f9 f9 f9 f9 07 f9 f9 f9
=>0x0aab94a5a1c0: f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9[05]f9 f9 f9
  0x0aab94a5a1d0: f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 05 f9 f9 f9
  0x0aab94a5a1e0: f9 f9 f9 f9 05 f9 f9 f9 f9 f9 f9 f9 07 f9 f9 f9
  0x0aab94a5a1f0: f9 f9 f9 f9 00 04 f9 f9 f9 f9 f9 f9 05 f9 f9 f9
  0x0aab94a5a200: f9 f9 f9 f9 07 f9 f9 f9 f9 f9 f9 f9 05 f9 f9 f9
  0x0aab94a5a210: f9 f9 f9 f9 05 f9 f9 f9 f9 f9 f9 f9 02 f9 f9 f9
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==80021==ABORTING

GitMensch

haven't checked if the error was triggered by that test case (I'd assume so) - but as the test is already run, please add a check for the expected value in Y (`if Y(1:) NOT = ") as well, then commit.

I'd say it is a follow-up of a previous recent change, so no need for a Changelog entry - but please reference it as "follow-up commit to r...." to point to the other one and give a minimal change note in the svn log.

ddeclerck · 2024-12-20T15:35:32Z

haven't checked if the error was triggered by that test case (I'd assume so)

At least it was, locally on my machine - and was fixed with the patch. So, let's be confident ;)

but as the test is already run, please add a check for the expected value in Y (`if Y(1:) NOT = ") as well, then commit.

Added.

I'd say it is a follow-up of a previous recent change, so no need for a Changelog entry - but please reference it as "follow-up commit to r...." to point to the other one and give a minimal change note in the svn log.

Alright.

ddeclerck · 2024-12-20T15:44:39Z

Merged in SVN @ 5407.

GitMensch requested changes Dec 9, 2024

View reviewed changes

ddeclerck force-pushed the fix_blank_zero branch from e3e46f7 to c2cd5f5 Compare December 9, 2024 15:13

GitMensch requested changes Dec 10, 2024

View reviewed changes

ddeclerck force-pushed the fix_blank_zero branch from ef2def5 to c57141f Compare December 12, 2024 17:17

ddeclerck mentioned this pull request Dec 12, 2024

Use global var for bdb_app_data #203

Closed

GitMensch reviewed Dec 12, 2024

View reviewed changes

libcob/ChangeLog Show resolved Hide resolved

libcob/move.c Outdated Show resolved Hide resolved

libcob/move.c Outdated Show resolved Hide resolved

libcob/move.c Outdated Show resolved Hide resolved

tests/testsuite.src/run_fundamental.at Outdated Show resolved Hide resolved

ddeclerck force-pushed the fix_blank_zero branch from c57141f to e4c78b8 Compare December 16, 2024 08:52

Fix BLANK WHEN ZERO on signed NUMERIC-EDITED

99c1c33

ddeclerck force-pushed the fix_blank_zero branch from e4c78b8 to bca3242 Compare December 16, 2024 13:25

GitMensch requested changes Dec 17, 2024

View reviewed changes

libcob/ChangeLog Show resolved Hide resolved

Use optimized_move_display_to_edited with all kind of sign positions

6d3e4fb

ddeclerck force-pushed the fix_blank_zero branch from bca3242 to 6d3e4fb Compare December 17, 2024 13:38

GitMensch approved these changes Dec 17, 2024

View reviewed changes

GitMensch added the Ready for SVN label Dec 17, 2024

GitMensch closed this Dec 17, 2024

GitMensch added Merged in SVN and removed Ready for SVN labels Dec 17, 2024

ddeclerck mentioned this pull request Dec 19, 2024

Dialect option: normalize BCD "on-the-fly" when moving from ALPHANUMERIC to NUMERIC #200

Open

GitMensch reopened this Dec 20, 2024

GitMensch added reopened (merged) already merged, but reopened and removed Merged in SVN labels Dec 20, 2024

GitMensch approved these changes Dec 20, 2024

View reviewed changes

Fix out of bounds read

8d6312b

ddeclerck force-pushed the fix_blank_zero branch from 2cffeca to 8d6312b Compare December 20, 2024 15:31

GitMensch added Merged in SVN and removed reopened (merged) already merged, but reopened labels Dec 20, 2024

GitMensch closed this Dec 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix BLANK WHEN ZERO on signed NUMERIC-EDITED #201

Fix BLANK WHEN ZERO on signed NUMERIC-EDITED #201

ddeclerck commented Dec 9, 2024

GitMensch left a comment

GitMensch Dec 9, 2024 •

edited

Loading

ddeclerck Dec 9, 2024

GitMensch Dec 9, 2024

ddeclerck Dec 10, 2024

GitMensch left a comment

ddeclerck commented Dec 12, 2024

GitMensch commented Dec 13, 2024

ddeclerck commented Dec 13, 2024 via email •

edited

Loading

GitMensch left a comment

ddeclerck commented Dec 17, 2024

GitMensch left a comment

ddeclerck commented Dec 17, 2024 •

edited

Loading

GitMensch commented Dec 20, 2024

GitMensch left a comment

ddeclerck commented Dec 20, 2024

ddeclerck commented Dec 20, 2024

Fix BLANK WHEN ZERO on signed NUMERIC-EDITED #201

Fix BLANK WHEN ZERO on signed NUMERIC-EDITED #201

Conversation

ddeclerck commented Dec 9, 2024

GitMensch left a comment

Choose a reason for hiding this comment

GitMensch Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

ddeclerck Dec 9, 2024

Choose a reason for hiding this comment

GitMensch Dec 9, 2024

Choose a reason for hiding this comment

ddeclerck Dec 10, 2024

Choose a reason for hiding this comment

GitMensch left a comment

Choose a reason for hiding this comment

ddeclerck commented Dec 12, 2024

GitMensch commented Dec 13, 2024

ddeclerck commented Dec 13, 2024 via email • edited Loading

GitMensch left a comment

Choose a reason for hiding this comment

ddeclerck commented Dec 17, 2024

GitMensch left a comment

Choose a reason for hiding this comment

ddeclerck commented Dec 17, 2024 • edited Loading

GitMensch commented Dec 20, 2024

GitMensch left a comment

Choose a reason for hiding this comment

ddeclerck commented Dec 20, 2024

ddeclerck commented Dec 20, 2024

GitMensch Dec 9, 2024 •

edited

Loading

ddeclerck commented Dec 13, 2024 via email •

edited

Loading

ddeclerck commented Dec 17, 2024 •

edited

Loading