Skip to content
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.

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

$) and $( should be IOKp but not IOK #18955

Closed
FGasper opened this issue Jul 1, 2021 · 4 comments
Closed

$) and $( should be IOKp but not IOK #18955

FGasper opened this issue Jul 1, 2021 · 4 comments

Comments

@FGasper
Copy link
Contributor

FGasper commented Jul 1, 2021

> perl -MDevel::Peek -e'Dump( my $v = $) )'
SV = PVMG(0x7fe9b782e380) at 0x7fe9b7806ad0
  REFCNT = 1
  FLAGS = (IOK,POK,IsCOW,pIOK,pPOK)
  IV = 20
  NV = 0
  PV = 0x7fe9b6505760 "20 20 505 504 12 61 79 80 81 98 33 100 204 250 395 398 399"\0
  CUR = 58
  LEN = 64
  COW_REFCNT = 1

The fact that this is marked IOK tells XS modules (e.g., Sereal) that the IV value is legitimate. That’s sort of true, but it’s inconsistent with Perl’s typical behaviour of invalidating IOK whenever the IV mismatches the PV, which is the case here.

Note that leaving the private flag (IOKp) in place will prevent not-a-number warnings.

@FGasper
Copy link
Contributor Author

FGasper commented Jul 1, 2021

This causes Sereal/Sereal#263

@atoomic
Copy link
Member

atoomic commented Jul 1, 2021

note that it's coming from the void)SvIOK_on(sv); /* what a wonderful hack! */ setting the IOK in all flags
view

perl5/mg.c

Line 1227 in c683824

case ')':

My understanding is the issue is when serializing the data.
We probably should balance pros/cons of having the IOK flag set there

FGasper added a commit to FGasper/perl5 that referenced this issue Jul 2, 2021
Issue Perl#18955: This will prevent serializers from serializing these
variables as numbers (which loses the additional groups).
FGasper added a commit to FGasper/perl5 that referenced this issue Jul 2, 2021
Issue Perl#18955: This will prevent serializers from serializing these
variables as numbers (which loses the additional groups).

This restores behaviour from 5.16.
FGasper added a commit to FGasper/perl5 that referenced this issue Jul 2, 2021
Issue Perl#18955: This will prevent serializers from serializing these
variables as numbers (which loses the additional groups).

This restores behaviour from 5.16.
FGasper added a commit to FGasper/perl5 that referenced this issue Jul 2, 2021
Issue Perl#18955: This will prevent serializers from serializing these
variables as numbers (which loses the additional groups).

This restores behaviour from 5.16.
FGasper added a commit to FGasper/perl5 that referenced this issue Jul 2, 2021
Issue Perl#18955: This will prevent serializers from serializing these
variables as numbers (which loses the additional groups).

This restores behaviour from 5.16.
FGasper added a commit to FGasper/perl5 that referenced this issue Jul 2, 2021
Issue Perl#18955: This will prevent serializers from serializing these
variables as numbers (which loses the additional groups).

This restores behaviour from 5.16.
@toddr
Copy link
Member

toddr commented Jul 2, 2021

Bisecting, you can look at:

LD_LIBRARY_PATH=/root/projects/perl ./perl -Ilib -MDevel::Peek -e'Dump( my $v = $) )' 2>&1|egrep '\bIOK\b'

If it gives back IOK, then the bug is still there.

At which point you can bisect it to:

4bac9ae47b5ad7845a24e26b0e95609805de688a is the first bad commit
commit 4bac9ae47b5ad7845a24e26b0e95609805de688a
Author: Chip Salzenberg <chip@pobox.com>
Date:   Fri Jun 22 15:18:18 2012 -0700
    Magic flags harmonization.
    In restore_magic(), which is called after any magic processing, all of
    the public OK flags have been shifted into the private OK flags.  Thus
    the lack of an appropriate public OK flags was used to trigger both get
    magic and required conversions.  This scheme did not cover ROK, however,
    so all properly written code had to make sure mg_get was called the right
    number of times anyway.  Meanwhile the private OK flags gained a second
    purpose of marking converted but non-authoritative values (e.g. the IV
    conversion of an NV), and the inadequate flag shift mechanic broke this
    in some cases.
    This patch removes the shift mechanic for magic flags, thus exposing (and
    fixing) some improper usage of magic SVs in which mg_get() was not called
    correctly.  It also has the side effect of making magic get functions
    specifically set their SVs to undef if that is desired, as the new behavior
    of empty get functions is to leave the value unchanged.  This is a feature,
    as now get magic that does not modify its value, e.g. tainting, does not
    have to be special cased.
    The changes to cpan/ here are only temporary, for development only, to
    keep blead working until upstream applies them (or something like them).
    Thanks to Rik and Father C for review input.
 av.c                             |   2 +-
 cpan/Compress-Raw-Bzip2/Bzip2.xs |  68 ++---
 cpan/Compress-Raw-Zlib/Zlib.xs   |  82 +++---
 doio.c                           |   1 +
 doop.c                           |  10 +-
 embed.fnc                        |   7 +-
 embed.h                          |   3 +-
 ext/Devel-Peek/t/Peek.t          |   2 +-
 gv.c                             |   2 +
 mg.c                             |  51 ++--
 pp.c                             |   2 +-
 pp_ctl.c                         |   8 +-
 pp_hot.c                         |  13 +-
 pp_sys.c                         |  19 +-
 proto.h                          |  20 +-
 sv.c                             | 558 ++++++++++++++++++---------------------
 sv.h                             | 169 ++++++------
 t/op/eval.t                      |   2 +-
 t/op/tie.t                       |   5 +-
 19 files changed, 486 insertions(+), 538 deletions(-)

nwc10 pushed a commit that referenced this issue Jul 6, 2021
Issue #18955: This will prevent serializers from serializing these
variables as numbers (which loses the additional groups).

This restores behaviour from 5.16.
@nwc10
Copy link
Contributor

nwc10 commented Jul 6, 2021

Fix is in blead.

@nwc10 nwc10 closed this as completed Jul 6, 2021
thibaultduponchelle pushed a commit to thibaultduponchelle/perl5 that referenced this issue Jul 9, 2021
Issue Perl#18955: This will prevent serializers from serializing these
variables as numbers (which loses the additional groups).

This restores behaviour from 5.16.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants