[CRIU] [Patch] Fix UB in choose_service_fd_base due to calling __builtin_clz(0)

[Radoslaw Burny]

It seems like patchwork is picking up the patches, but it's unhappy with
the formatted corruption caused by my Vim config
(basically, tabs were rendered as spaces and caused merge conflicts when
included in the patch).
Let me try one last time before I'm banned from this mailing list for spam
:)



From: Radoslaw Burny <rburny at google.com>

Subject: [PATCH] Fix UB in choose_service_fd_base.

Signed-off-by: Radoslaw Burny <rburny at google.com>

---
 criu/util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/criu/util.c b/criu/util.c
index b19bf517..48ba09a8 100644
--- a/criu/util.c
+++ b/criu/util.c
@@ -588,9 +588,9 @@ static int choose_service_fd_base(struct pstree_item
*me)
  nr += 16; /* Safety pad */
  real_nr = nr;

- nr /= (1024 / sizeof(void *));
+ /* Align nr to the power of 2 for easier debugging */
+ BUG_ON(nr <= 0);
  nr = 1 << (32 - __builtin_clz(nr));
- nr *= (1024 / sizeof(void *));

  if (nr > service_fd_rlim_cur) {
  /* Right border is bigger, than rlim. OK, then just aligned value is
enough */
-- 
2.17.0.484.g0c8726318c-goog

On Mon, Apr 9, 2018 at 6:19 PM, Kirill Tkhai <ktkhai at virtuozzo.com> wrote:

> On 09.04.2018 19:16, Radoslaw Burny wrote:
> > Ah, I see. Even with the fds that CRIU adds, it's still possible to fit
> > below 64 or even 32.
> > I've reverted the patch to the version you suggested and attached it
> below
> > - I presume this is the code review process for CRIU, right?
>
> I assume, criu patchwork will be able to pick this patch up from the
> reply. Andrew, is this OK?
>
> > BTW, if you want to make any changes to the patch before merging it -
> > feel free to do so :)
> > It will be faster than us two iterating over the email.
> >
> > Thanks!
>
> Thanks,
> Kirill
>
> > On Mon, Apr 9, 2018 at 6:03 PM, Kirill Tkhai <ktkhai at virtuozzo.com>
> wrote:
> >
> >> On 09.04.2018 17:57, Radoslaw Burny wrote:
> >>> From: Radoslaw Burny <rburny at google.com>
> >>>
> >>> Subject: [PATCH] Fix UB in choose_service_fd_base.
> >>>
> >>> Signed-off-by: Radoslaw Burny <rburny at google.com>
> >>>
> >>> ---
> >>>  criu/util.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/criu/util.c b/criu/util.c
> >>> index b19bf517..48ba09a8 100644
> >>> --- a/criu/util.c
> >>> +++ b/criu/util.c
> >>> @@ -588,9 +588,9 @@ static int choose_service_fd_base(struct
> pstree_item
> >>> *me)
> >>>         nr += 16; /* Safety pad */
> >>>         real_nr = nr;
> >>>
> >>> -       nr /= (1024 / sizeof(void *));
> >>> +       /* Align nr to the power of 2 for easier debugging */
> >>> +       BUG_ON(nr <= 0);
> >>>         nr = 1 << (32 - __builtin_clz(nr));
> >>> -       nr *= (1024 / sizeof(void *));
> >>>
> >>>         if (nr > service_fd_rlim_cur) {
> >>>                 /* Right border is bigger, than rlim. OK, then just
> >> aligned
> >>> value is enough */
> >>
> >> It's not a round up power of 2, it's round up power of 2 rounded to 128.
> >> This goes from kernel: https://git.kernel.org/pub/
> >> scm/linux/kernel/git/torvalds/linux.git/tree/fs/file.c#n85
> >>
> >> #include <limits.h>
> >> int main(void)
> >> {
> >> unsigned int i, was, nr;
> >>
> >>     for (i = 0; i < INT_MAX; i++) {
> >>             nr = i;
> >>             nr /= (1024 / sizeof(void *));
> >>             if (nr)
> >>                     nr = 1 << (32 - __builtin_clz(nr));
> >>             else
> >>                     nr = 1;
> >>             nr *= (1024 / sizeof(void *));
> >>
> >>             printf("nr=%d, i=%d\n", nr, i);
> >>             if (nr < i || (nr-1) & nr) {
> >>                     printf("error\n");
> >>                     exit(1);
> >>             }
> >>     }
> >>
> >>     return 0;
> >>
> >> }
> >>
> >> nr=128, i=16
> >> nr=128, i=17
> >> nr=128, i=18
> >> nr=128, i=19
> >> nr=128, i=20
> >> nr=128, i=21
> >> nr=128, i=22
> >> nr=128, i=23
> >> nr=128, i=24
> >> nr=128, i=25
> >> nr=128, i=26
> >> nr=128, i=27
> >> nr=128, i=28
> >> nr=128, i=29
> >> nr=128, i=30
> >>
> >
> >
> >
> > From: Radoslaw Burny <rburny at google.com>
> >
> >
> >
> > Subject: [PATCH] Fix UB in choose_service_fd_base (rev 2).
> >
> >
> >
> >
> >
> > Signed-off-by: Radoslaw Burny <rburny at google.com>
> >
> >
> >
> > ---
> >  criu/util.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/criu/util.c b/criu/util.c
> > index b19bf517..56a1683e 100644
> > --- a/criu/util.c
> >
> > +++ b/criu/util.c
> >
> > @@ -589,7 +589,10 @@ static int choose_service_fd_base(struct pstree_item
> > *me)
> >         real_nr = nr;
> >
> >         nr /= (1024 / sizeof(void *));
> > -       nr = 1 << (32 - __builtin_clz(nr));
> > +       if (nr != 0)
> > +               nr = 1 << (32 - __builtin_clz(nr));
> > +       else
> > +               nr = 1;
> >         nr *= (1024 / sizeof(void *));
> >
> >         if (nr > service_fd_rlim_cur) {
> > --
> > 2.17.0.484.g0c8726318c-goog
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/criu/attachments/20180409/55e7baea/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4843 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.openvz.org/pipermail/criu/attachments/20180409/55e7baea/attachment-0001.p7s>