-
Notifications
You must be signed in to change notification settings - Fork 70
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
strncpy warnings #151
Comments
Please include compiler version and full output from ./configure etc |
Full R CMD check report at https://win-builder.r-project.org/incoming_pretest/haven_2.0.0_20181009_004546/Debian/00check.log — I'll try and figure out how to get the other info |
Oh duh, from simple URL manipulation:
Oh nope, that's not |
Should be GCC 8.2.0 |
Is -Wno-stringop-truncation an option for you? Some of this code is written to use Pascal strings (unterminated string + length).
… On Oct 12, 2018, at 07:03, Hadley Wickham ***@***.***> wrote:
Should be GCC 8.2.0
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#151 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIONwWErChpJnisnUwWpf-VCXBh4aitks5ukKEhgaJpZM4XNzrk>.
|
If that's the case, why not use |
|
At the very end of https://developers.redhat.com/blog/2018/05/24/detecting-string-truncation-with-gcc-8/ it says:
Would you accept a patch that decorated the pascal strings with |
Fine with me, provided everything works with Clang too. |
Before I do this, I just wanted to double-check that we definitely can't just switch to It seems like it would be safe in both of the following two common cases: if (label && strlen(label)) {
value_label->label_len = strlen(label);
value_label->label = malloc(value_label->label_len);
strncpy(value_label->label, label, value_label->label_len);
}
...
strncpy(header_start.file_label, writer->file_label, sizeof(header_start.file_label)); |
As I stated earlier, |
I just wanted to double check that it's necessary - as far as I can tell most uses of |
i.e. (and I totally accept that I'm probably just being particularly dense) I can't see where the zero-fill behaviour is needed in the above code |
This line zero-fills
|
Ah got it. I'm still having problems replicating the compiler warnings locally, so haven't yet got to the point of trying out fixes 😞 |
Try |
That's an improvement, but I still see a few:
You can see my (likely feeble!) efforts to fix the problem at tidyverse/haven#460 (I was trying to prototype on my embedded of readstat before I bothered you for more feedback; but it might be useful). We also discovered that to see the full set of warnings that I'm getting from CRAN you need |
Please try the referenced commit (dev branch, not master) |
Oh shoot, I'm an idiot - sorry! |
Now I only see one warning:
|
Ok try 369e51a |
Well, that broke Travis. Here's another stab at it: 17e912b |
That did it! Is this branch ok to use in the haven release? I'm trying to get something to CRAN today/tomorrow before I leave for a conference. |
It should be fine right now, but generally |
The text was updated successfully, but these errors were encountered: