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

Fix #3276: Remove two major defects in posixlib utsname.scala #3280

Merged

Conversation

LeeTibbert
Copy link
Contributor

@LeeTibbert LeeTibbert commented May 8, 2023

Fix #3276.

I theory, this PR, is not a candidate for backport. However, the code in SN 0.4.n is
so broken that a backport could not be accused of being a breaking change.

Two major defects in utsname are removed:

  1. the uname method can not longer write to Scala Native memory that it
    does not "own".

  2. fields in the utsname structure after the first, say nodename or version now
    contain valid data as returned by the OS. Previously they had most likely been
    empty strings.

An entry was made in the 0.5.0 changelog which should cover this PR and the prior
PR #3264 by mox692.

My personal thanks to mox692 for shining the light on utsname and causing
detailed review.

@LeeTibbert
Copy link
Contributor Author

LeeTibbert commented May 8, 2023

The one failure is in Windows initialization code and has nothing to do with the
substance of this PR.

All else is Green, like Kermit.

@LeeTibbert
Copy link
Contributor Author

@mox692
If you have time & interest, could you take a look at this PR and tell me what you think?
It is alway nice to have a second pair of eyes on code. Thank you. I understand if the press of
events take you elsewhere. I have a bit of that going on myself.

Is it consistent with what you set out to do?

@ekrich
Copy link
Member

ekrich commented May 8, 2023

I was wondering if we have to model the struct as a set of CArrays? It seems that if the POSIX spec says the byte array is null terminated that it might just make sense to model it as CStrings. To me this seems like it would avoid many of the foot shots and all the casting issues. Using a good Scaladoc could explain it. I know this is not the exact mapping of the spec but do we have to do that if we have good reason?

@mox692
Copy link
Contributor

mox692 commented May 9, 2023

@LeeTibbert
Thank you for updating
Sure, I'll look at it after work today :)

@LeeTibbert LeeTibbert changed the title Partial fix #3276: Fix two major defects in posixlib utsname.scala Fix #3276: Fix two major defects in posixlib utsname.scala May 9, 2023
@LeeTibbert
Copy link
Contributor Author

Eric, thank you for the discussion. It is always worthwhile to ask if one is inside
the box because that is the better place to be.

I think that leaving the CArray declarations as is is the the better way to go. Staying
close to both the Open Group description and to what Linux & macOS actually allocate
reduces complexity and divergence.

I put a note in the utsname.scala file describing how one might convert the null-containing
CArrays into proper Scala Strings. I think that will make the easier to use.

@LeeTibbert LeeTibbert changed the title Fix #3276: Fix two major defects in posixlib utsname.scala Fix #3276: Remove two major defects in posixlib utsname.scala May 9, 2023
Copy link
Contributor

@mox692 mox692 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @LeeTibbert.
I left just one question, but other part looks ok to me.


#define SIZEOF_FIELD(t, f) (sizeof(((t *)0)->f))

_Static_assert(SIZEOF_FIELD(struct scalanative_utsname, sysname) <=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, shouldn't we write
SIZEOF_FIELD(struct utsname, sysname)
instead of
SIZEOF_FIELD(struct scalanative_utsname, sysname) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mox692 You are absolutely correct. Good catch, thank you.

Fixed now.

@WojciechMazur WojciechMazur merged commit 0fb6ba1 into scala-native:main May 12, 2023
68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

posixlib utsname is hard broken & potentially dangerous to memory.
4 participants