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 segfaults when assigning to list column #4350

Merged
merged 10 commits into from
May 18, 2021

Conversation

tlapak
Copy link
Contributor

@tlapak tlapak commented Apr 3, 2020

Closes #4166
Closes #4667
Closes #4678
Closes #4729

The issue there was unrelated to the unlisting. A minimal example that also segfaults is

dt <- data.table(a=list(1:2, 3, 4))
dt[, a:=1:4]

Turns out when LHS is a list column and RHS isn't, no length checks were being made. I am slightly surprised by @MichaelChirico's result which was maybe even intended.

While testing my fix I noticed that subassigning a vector of correct length also segfaults:

dt <- data.table(a=list(1:2, 3, 4))
dt[1:2, a:=1:2]

I see three possible ways of handling this:

  1. Error out with a coercion error
  2. Treat RHS as length 1 and replicate
  3. Coerce to list by wrapping elements of RHS in list()

I opted for 3. because why not. It seems the most intuitive to me besides 1. But happy to change this if people disagree. Required 'some' abuse of the BODY macro in memrecycle...

@codecov
Copy link

codecov bot commented Apr 3, 2020

Codecov Report

Merging #4350 (5a811d1) into master (0cbc418) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4350   +/-   ##
=======================================
  Coverage   99.47%   99.47%           
=======================================
  Files          75       75           
  Lines       14775    14785   +10     
=======================================
+ Hits        14697    14707   +10     
  Misses         78       78           
Impacted Files Coverage Δ
src/assign.c 99.71% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cbc418...5a811d1. Read the comment docs.

@tlapak
Copy link
Contributor Author

tlapak commented Apr 3, 2020

Uh, if I understand the Codecov report correctly, I need to add unit tests for raw and complex rhs and one that doesn't fit any of these types?

@tlapak tlapak closed this Apr 3, 2020
@tlapak tlapak reopened this Apr 3, 2020
@jangorecki
Copy link
Member

Thanks for PR. Yes, every new line of C has to be reached for code coverage to be happy. We can always workout code coverage later on. Much more important to pass the CI checks.

@tlapak
Copy link
Contributor Author

tlapak commented Apr 3, 2020

Cool, thanks for the explanation. And I will try not to fat finger the close button again.

@jangorecki
Copy link
Member

if I understand the Codecov report correctly, I need to add unit tests for raw and complex rhs and one that doesn't fit any of these types?

Yes, you understand correct. If you would push to Rdatatable org branch, it would be much easier for others to add those tests for you. If you cannot yet push branches to our org then @mattdowle should be able to invite you to members.

@tlapak
Copy link
Contributor Author

tlapak commented Apr 12, 2020

I'm not a member so can't push to this repo.

Also, sorry for the delay. My time is a bit more limited than usual atm. I've now added the missing unit tests and found and fixed a bug in the process. I'm sure there's a lesson in there somewhere.

@mattdowle mattdowle merged commit eb9337d into Rdatatable:master May 18, 2021
@tlapak tlapak deleted the fix_segfault_overwriting_listcol branch May 18, 2021 11:06
@jangorecki jangorecki removed this from the 1.14.9 milestone Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants