limit the field width of 'scanf'

[Daz-3ux]

Fixes: #2121

limit the field width of 'scanf'

[Snorch]

We might also need to fix

• sscanf in parse_mountinfo_ent
• sscanf in test/zdtm/static/change_mnt_context.c
• sscanf in test/zdtm/static/file_locks01.c

[Daz-3ux]

Copy that, I'll fix these ASAP.

[Snorch]

From code perspective your patches look good to me.

But we need some more changes:

• Please squash your commits in one as logically they belong together (it is especially obvious with parse_file_lock_buf hunks =) ).
• Fix dco checks, you should put your signed-off-by properly https://github.com/checkpoint-restore/criu/pull/2123/checks?check_run_id=11989951339

Author: Daz-3ux <[email protected]>
Signed-off-by: Pengda Yang <daz-3ux at proton.me>

The above fields should be the same.

You would probably need to learn some more about git --amend, --author, interactive rebase and force push to acomplish it.

[Daz-3ux]

From code perspective your patches look good to me.

But we need some more changes:

• Please squash your commits in one as logically they belong together (it is especially obvious with parse_file_lock_buf hunks =) ).
• Fix dco checks, you should put your signed-off-by properly https://github.com/checkpoint-restore/criu/pull/2123/checks?check_run_id=11989951339

Author: Daz-3ux <[email protected]>
Signed-off-by: Pengda Yang <daz-3ux at proton.me>

The above fields should be the same.

You would probably need to learn some more about git --amend, --author, interactive rebase and force push to acomplish it.

I think I've fixed the DCO error, is everything alright now? :)

[Snorch]

Seems that the change in parse_mountinfo_ent introduced this asan error:

 ==11236==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x621000003900 at pc 0x7f6b4530f3b5 bp 0x7ffcfb06db10 sp 0x7ffcfb06d2c0
WRITE of size 4096 at 0x621000003900 thread T0
    #0 0x7f6b4530f3b4 in scanf_common(void*, int, bool, char const*, __va_list_tag*) (/lib64/libasan.so.8+0x733b4)
    #1 0x7f6b4530ffce in __isoc99_vsscanf (/lib64/libasan.so.8+0x73fce)
    #2 0x7f6b453100be in __isoc99_sscanf (/lib64/libasan.so.8+0x740be)
    #3 0x53b828 in parse_mountinfo_ent criu/proc_parse.c:1410
    #4 0x545a1f in parse_mountinfo criu/proc_parse.c:1632
    #5 0x4f02d3 in collect_mntinfo criu/mount.c:1885
    #6 0x4f0409 in collect_mntns criu/mount.c:3954
    #7 0x4fd5bb in collect_mnt_namespaces criu/mount.c:3974
    #8 0x509919 in collect_namespaces criu/namespaces.c:1610
    #9 0x474ea7 in cr_dump_tasks criu/cr-dump.c:2192
    #10 0x42e470 in main criu/crtools.c:289
    #11 0x7f6b44d7450f in __libc_start_call_main (/lib64/libc.so.6+0x2750f)
    #12 0x7f6b44d745c8 in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x275c8)
    #13 0x42f8c4 in _start (/criu/criu/criu+0x42f8c4)
    ```
    https://github.com/checkpoint-restore/criu/actions/runs/4424844531/jobs/7777804713

[Daz-3ux]

Seems that the change in parse_mountinfo_ent introduced this asan error:

 ==11236==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x621000003900 at pc 0x7f6b4530f3b5 bp 0x7ffcfb06db10 sp 0x7ffcfb06d2c0
WRITE of size 4096 at 0x621000003900 thread T0
    #0 0x7f6b4530f3b4 in scanf_common(void*, int, bool, char const*, __va_list_tag*) (/lib64/libasan.so.8+0x733b4)
    #1 0x7f6b4530ffce in __isoc99_vsscanf (/lib64/libasan.so.8+0x73fce)
    #2 0x7f6b453100be in __isoc99_sscanf (/lib64/libasan.so.8+0x740be)
    #3 0x53b828 in parse_mountinfo_ent criu/proc_parse.c:1410
    #4 0x545a1f in parse_mountinfo criu/proc_parse.c:1632
    #5 0x4f02d3 in collect_mntinfo criu/mount.c:1885
    #6 0x4f0409 in collect_mntns criu/mount.c:3954
    #7 0x4fd5bb in collect_mnt_namespaces criu/mount.c:3974
    #8 0x509919 in collect_namespaces criu/namespaces.c:1610
    #9 0x474ea7 in cr_dump_tasks criu/cr-dump.c:2192
    #10 0x42e470 in main criu/crtools.c:289
    #11 0x7f6b44d7450f in __libc_start_call_main (/lib64/libc.so.6+0x2750f)
    #12 0x7f6b44d745c8 in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x275c8)
    #13 0x42f8c4 in _start (/criu/criu/criu+0x42f8c4)
    ```
    https://github.com/checkpoint-restore/criu/actions/runs/4424844531/jobs/7777804713

My bad, I will check it

[Daz-3ux]

@Snorch Did I fix the bug？ sorry I am not familiar with the test process

[Snorch]

There are linter errors, but patch looks better (cleaner) for me without fixing them. Let's see what others would say.

[codecov-commenter]

Codecov Report

Patch coverage: 48.83% and project coverage change: -0.61 ⚠️

Comparison is base (4666539) 70.94% compared to head (37d3ea9) 70.33%.

❗ Current head 37d3ea9 differs from pull request most recent head ed00365. Consider uploading reports for the commit ed00365 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@             Coverage Diff              @@
##           criu-dev    #2123      +/-   ##
============================================
- Coverage     70.94%   70.33%   -0.61%     
============================================
  Files           133      131       -2     
  Lines         32646    33853    +1207     
============================================
+ Hits          23160    23812     +652     
- Misses         9486    10041     +555     

Impacted Files	Coverage Δ
criu/cr-check.c	61.71% <0.00%> (ø)
criu/include/util.h	89.65% <ø> (-10.35%)	⬇️
criu/util.c	64.44% <0.00%> (ø)
criu/proc_parse.c	68.57% <66.66%> (ø)
criu/net.c	76.65% <73.07%> (-0.03%)	⬇️

... and 26 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[yew011]

On Sun, Apr 2, 2023 at 7:02 PM Aritra Mallick ***@***.***> wrote: ***@***.**** approved this pull request. — Reply to this email directly, view it on GitHub <#2123 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAS4DEQ6YPF76NWYAF6TR23W7IVURANCNFSM6AAAAAAV2EWPQU> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- Alex Wang, Open vSwitch developer

[Snorch]

It is decided to take this patch as is with "magic numbers" for now but delay it to next release of master. Let's reconsider "magic numbers" problem when we would be preparing next release.

[rst0git]

Let's reconsider "magic numbers" problem when we would be preparing next release.

@Snorch should we create a GitHub issue to keep track and do you have any suggestions on how to address this problem?

[Snorch]

@rst0git done. #2154